fabricjs / fabric.js

Javascript Canvas Library, SVG-to-Canvas (& canvas-to-SVG) Parser
http://fabricjs.com
Other
28.75k stars 3.49k forks source link

Object not selectable after moving though event #1722

Closed dctr closed 9 years ago

dctr commented 9 years ago

I have the following event based moving to align the door when the corresponding wall moves:

      wall.on('modified', function wallsModified() {
        repositionDoor();
      });

      repositionDoor = function repositionDoor() {
        // calc px, py, a depending on wall position
        door.set('left', px);
        door.set('top', py);
        door.set('angle', a);
      };

BTW: Door is a thick line, while wall is a rect.

Before moving, the door can be moved by mouse drag and drop. After it is not selectable anymore.

I tried canvas.setActiveObject(door) and the door gets controls, but is not movable anyway.

dctr commented 9 years ago

This is actually the same behaviour as issue #1717: The selection area is on the old position.

asturur commented 9 years ago

Hi @dctr could you try to setup a little jsFiddle for us to test the issue?

dctr commented 9 years ago

http://jsfiddle.net/1e8kztaf/3/

=> rect moves => to drag and drop th rect, you have to use the old position

asturur commented 9 years ago

fiddle looks incomplete. I don't have any rect, there is no code.

ok found http://jsfiddle.net/1e8kztaf/3/

dctr commented 9 years ago

Is there any possible workaround? A dirty hack would do too. I have to finish the project soon...

asturur commented 9 years ago

done, call setCoords as i did. I don't know what is it and why we need it, but i think is in the docs and has already been said somewhere.

http://jsfiddle.net/1e8kztaf/4/

If this solves both of your issue, please close them both. regards.

dctr commented 9 years ago

That solves it, thank you. In my opinion it is error prone to make those properties public. If you instead would have to call

        door.setCoords({
          left: positionX,
          top: positionY
        });

(which actually does not work, neither with left/top nor with x/y) this update could be handled by fabric.

If possible, i'd like to keep this bug open and tag it as feature request to remove those public properties.

Kienz commented 9 years ago

If you change position properties you have to call object.setCoords() to recalcualte the coordinates of the boundingBox. Otherwise the boundingBox coordinates stay at the old position. http://fabricjs.com/docs/fabric.Object.html#set

asturur commented 9 years ago

I don't know. I'm new to fabricJs development and to javascrit class based development in general. For sure public and private does not make sense in javascript, and should be better to stick to the docs when available. Happy to help you anyway.

dctr commented 9 years ago

@asturur: yes there is such a thing as making things private (through closures) but fabric uses the "classical oo" apprach. Yet, in this approach it is common to just use an unserscore to pretend its private. This would, in my optionion, be feasable.

@Kienz: As properties are public anyway, i did not consult the "set" documentation.

kangax commented 9 years ago

@dctr This is mostly about performance. I don't think this has to do with private vs. public. set is a public API; so is setCoords. Both can be used separately. However, if you need to update "visual" controls (or rather — their area for mouse/touch detection), setCoords does just that. On the other hand, if you wish to update hundreds or thousands of objects on screen (their position/angle/scale), you wouldn't want set to call setCoords under the hood, killing performance significantly.

Having said this, we could provide an option for all set methods to call setCoords as a convenience, for cases when you don't care about performance (perhaps operating on small number of objects). Would this make things clearer though? I'm not sure. As someone not familiar with Fabric's mechanics, you'd still need to know to "activate" that convenience mode; you might as well just call setCoords after set :)

dctr commented 9 years ago

I think implicitly calling setCoords for set('x', val); would undermine the current schema. As not all set() methods should update the coords, it would be an unexpected side-effect.

Furthermore, i get you point regarding the performance.

Finally, if you know the API better, you get to know setCoords() someday.

FabricJS is just designed that way. In a rather closed scenario, there might only be a udpateCoords({x: val1, y: val2});, no set('x', val); or object.x = val; at all. In FabricJS, you just can write everythink and have to handle updates youself. Thats OK once you know it.

Thanks for all your support.

cancerberoSgx commented 5 years ago

Just in case, the setCoords() suggestion save my day, using latest version of the library and moving / assigning left, top, width, height properties of an object programmatically. . from my point of view (new user) this was kind of painful and unexpected. what I can understand is that methods like canvas.renderAll should recalculate everything. Is there any method which forces all the aspects of all shapes ? Also the same for the canvas itself with respect of the DOM ? I would like to contribute with a little note in the docs so this gain more visibility, but I wonder if as a new user I' not missing some important documentation .. I was looking to the API but is quite extensive to grasp this kind of details while getting started. Thanks!

asturur commented 5 years ago

Hi @cancerberoSgx i just uploaded a small page where indeed i explain the most common mistakes for starters due to some non intuitive api.

http://fabricjs.com/fabric-gotchas

if you find more, please advice. I m also rewriting the introduction to add more knowledge and updates that came in the years of updates.

cancerberoSgx commented 5 years ago

Thanks man this project is really awesome, I'm building a bitmap processing visual editor using opencv.js (computer vision library) an a port of mine of Imagemagick and its delegate libraries (both webassembly ports). Those two provides excellent support for bitmap processing . fabric.js is being really really useful to build, on top of the bitmap, the "editor GUI" . As mentioned in https://github.com/fabricjs/fabric.js/issues/5861 I also implemented rectangle/ellipse/line free drawing support for fabric and it's looking really good although I''m new with the library. I will share a demo when I have something descent and interesting. BTW I'm planning to release the "shape free drawing " as a separate project - ideally I would like to PR here but I don't know If I will have time to get familiar with the internals - nevertheless I would like share my progress with you here.Thanks for the gotchas and the library! keep it up

cancerberoSgx commented 5 years ago

if you find more, please advice. I m also rewriting the introduction to add more knowledge and updates that came in the years of updates.

Only a very small one, it's about programmatically moving the canvas itself in the document's viewport. If you change the style properties of the original canvas it will break the app. They safe way I found it is to target the containerEl which fortunately is accesible since its class name is available (I think canvas.containerElClass). Although I'm not sure if this is important for normal users - right now the only feedback I remember give me some pain.

asturur commented 5 years ago

what style properties did you change? can you make a practical example just so that i can understand if is uncharted territory or if something that already happened

cancerberoSgx commented 5 years ago

now that I understand that fabric creates another canvas to implement the "micro interactions" I see what happened to me was logic:

var canvas = document.getElementById('foo')
var fcanvas = new fabric.Canvas(canvas, {...})
...  create some shapes
...
canvas.style.paddingTop='200px'

stops working - the two canvas are out of sync...

This happened to me while just starting, and was confusing since I didn't imagine the implementation adds another canvas on top of mine. Also,I remember being kind of disappointed that the library reorganized the DOM structure and reparent my canvas . In my case I want to isolate the canvas from any external overhead leaving the canvas outside the react tree of components, and not using any fancy CSS or JS library

And BTW , right now I'm performing the video/image processing in a separate canvas than fabric's putting it on top of mine with transparent background that present only the "editor GUI". Fortunately I'm not seeing any detriment on the image processing performance on my side :)

These are some examples of what I'm talking about (no fabric.js on that ones yet)

At some point I would want to experiment with the technologies together in the same "document". for example could be easier to do simple / common tasks like transformations, basic convolutions/color filters with fabric and complex stuff like feature detection, classification video processing with the others. If/when I reach to that point probably I will end up using a 4th canvas to mix it all together. (Any recommendations ideas here are welcome).

With fabric right now I'm trying to make an interactive editor for the grab-cut background removal (example in the playground f the first two links) where user define the "regions of interest" and the background areas drawing rectangles (and probably also using a brush-like tool also toe refine it even more. Thanks - any feedback is appreciated, thanks again

asturur commented 5 years ago

Well eventually you can write your own objects to handle that. The fabric.Image object for example can handle any drawable. you can assign as element any other canvas that is processed by other code and have fabricJS just drag it around and scale or rotate it. Videos too are good.

Also fabricJS filters are meant to handle operations, nothing stops you from writing a filter that hands off the imageData to another processing and put it back at the end of processing.

The input of warning devs clearly of the double canvas is good, i should add it.

Other than that the upperCanvas was doing little when i joined the project. Today is handlinf text selection and cursor, drag to select and brushes. Soon enough i m planning to move controls rendering there so that selections of objects does not require a redraw and so that partial re rendering of objects is not affected from controls geometry.

cancerberoSgx commented 5 years ago

Well eventually you can write your own objects to handle that. The fabric.Image object for example can handle any drawable. you can assign as element any other canvas that is processed by other code and have fabricJS just drag it around and scale or rotate it. Videos too are good

That would be perfect - didn't saw an example with a video component. If I continue this path and there is none I will try to contribute as an example. Nevertheless I have some doubts about what the implementation should look like- mostly to support z-index which is kind of basic. I mean videos can be rendered in a rectangle and be transformed anyway but I don't know if the library is responsible of coordinating rendering time and if so I don't know if it's a good idea. .Or it doesn't participate in that and each component implementation free and responsible of writing the data ? Anyway I perhaps implementing layers by just using overlapped canvas (leaving the work to the browser visual engine is the best) (just thinking aloud) ...