Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support css-style zIndex #5088

Closed
wants to merge 6 commits into from
Closed

Support css-style zIndex #5088

wants to merge 6 commits into from

Conversation

maxhudson
Copy link

canvas.moveTo/object.moveTo as a solution to zPosition/zIndex doesn't really work when you have more than a couple objects on the canvas because moving any object around in the array affects the index of all other objects in the array.

This solution shouldn't affect the order of the array unless zIndex values are set on objects, so it shouldn't break sendToBack, sendToFront, etc...

It allows multiple objects to have the same zIndex value and when that is the case, leave them in their natural order.

@asturur
Copy link
Member

asturur commented Jul 6, 2018

This change would cause breaking functionality for bringForward backward, back and top.

It cannot coexist with the actual implementation.

If you want to complete the code inserting zIndex export, svg export, compatibility with z-stack functions, we can open a repository for addons and put it there. But it can't fit in the core library since is an extra feature that i m worried will cause incompatibility with old code more than benefits.

Can you also explain to what extent canvas.moveTo/object.moveTo as a solution to zPosition/zIndex doesn't really work when you have more than a couple objects on the canvas because moving any object around in the array affects the index of all other objects in the array. ?

did you find a bug or a weird use case?

@maxhudson
Copy link
Author

@asturur that makes sense about it breaking bring/send functions - but it only breaks them if you try to mix and match bring/send with zIndex, which would be weird to do.

.moveTo() pulls an item from the array (shifting the 'index' of every following shape) and then inserts an item into the array (again, shifting every following index). Three .moveTo operations and you have a pretty tricky scenario to keep in your head. Combine that with continuously adding shapes and it 10 or so layers that need to retain their order on the z-plane and you've got a proper nightmare on your hands.

My solution allows each of those 10 layers to be passed a zIndex between 0 and 9, and it just works.

Plugin makes sense in terms of isolating negative impact, but this is seems like pretty core behavior that just hasn't been flushed out.

@asturur
Copy link
Member

asturur commented Jul 6, 2018

Well but still i can't figure out, how moveTo does not work.

What is the application feature that is hard to implement with moveTo? can you make a practical example?

how would you implement bringForward with zIndex?
how would you put a layer between 2 that have the same zindex?

@maxhudson
Copy link
Author

maxhudson commented Jul 6, 2018

@asturur all moveTo does is take an object and move it around inside this._objects.

Example:

var object1, object2, object3
//add all objects to canvas
//canvas._objects = [object1, object2, object3];
object1.moveTo(1);
//canvas._objects = [object2, object1, object3];
object3.moveTo(1);
//canvas._objects = [object2, object3, object1];

now object1 is no longer at index 1. This compounds once this happens a few times. It's also hard to keep up to date as you add new items.

Imagine we have three layers:

var layerAObjects = [o1, o2, o3], layerBObjects = [o4, o5, o6], layerCObjects = [o7, o8, o9];
//add all objects to canvas
//canvas._objects = [o1, o2, o3, o4, o5, o6, o7, o8, o9];
//say I want layer order to be C, B, A...
layerCObjects.forEach(object => object.moveTo(0)); //O(n * 3) where n is total number of objects, and this reorders canvas._objects
layerBObjects.forEach(object => object.moveTo(0)); //O(n * 3)
layerAObjects.forEach(object => object.moveTo(0)); //O(n * 3)
//then I want to change it to B, C, A...
layerBObjects.forEach(object => object.moveTo(0)); //O(n * 3)
layerCObjects.forEach(object => object.moveTo(0)); //O(n * 3)
layerAObjects.forEach(object => object.moveTo(0)); //O(n * 3)

You basically have to move every object in the array to reliably reorder it with a large number of objects. You can try to use real values instead of 0, but it gets very complex.

With zIndex, it's faster, more clear what is happening, and it's not a question of whether or not objects will appear in the correct order:

var layerAObjects = [o1, o2, o3], layerBObjects = [o4, o5, o6], layerCObjects = [o7, o8, o9];
//add all objects to canvas
//canvas._objects = [o1, o2, o3, o4, o5, o6, o7, o8, o9];
//say I want layer order to be C, B, A...
layerCObjects.forEach(object => object.set({zIndex: 0})); //O(3)
layerBObjects.forEach(object => object.set({zIndex: 1})); //O(3)
layerAObjects.forEach(object => object.set({zIndex: 2})); //O(3)
//then I want to change it to B, C, A...
layerBObjects.forEach(object => object.set({zIndex: 0})); //O(3)
layerCObjects.forEach(object => object.set({zIndex: 1})); //O(3)
layerAObjects.forEach(object => object.set({zIndex: 2})); //O(3)

My use case is much more complex than this, with ~10 models that each have multiple canvas objects and the models need to appear on the canvas an order dependent on perspective.

@asturur
Copy link
Member

asturur commented Jul 6, 2018

You are just moving the code to something that is easier for you and not generally better.

if you want to move object in blocks yo can still use normal array operations right?

what is wrong with

canvas._objects = [].concat(layerC, layerB, layerA) This will exactly to the same of

layerCObjects.forEach(object => object.set({zIndex: 0})); //O(3)
layerBObjects.forEach(object => object.set({zIndex: 1})); //O(3)
layerAObjects.forEach(object => object.set({zIndex: 2})); //O(3)

with the O notation very low i guess.

@maxhudson
Copy link
Author

@asturur I don't think I am, but I see why you think that.

You are correct that your concat solution works in my simplified scenario, but what if I want to add an object to 'layer b'? Or move just one layer around? I have to reconstruct the entire array like you're doing every time.

Are you familiar with css z-index? fabric's moveTo solution is like css not having z-index: if it didn't, developers would have to track and restructure the dom hierarchy every time they wanted to manipulate z-index. It's the difference between picking a number to indicate when something should be displayed and restructuring the entire underlying system so that it displays in order every time something changes. That difference is more stark with more complexity (it gets more an more confusing mentally with more objects/layers, where as zIndex is pretty much linear) and it comes up whenever you add new objects, or move individual or groups of objects around in hierarchy.

@kangax can you take a look at this please?

#135

@maxhudson
Copy link
Author

Regardless, it does appear that my solution is incomplete in at least one area (inside findTarget/_searchPossibleTargets - doesn't sort objects by zIndex). No doubt there are other areas as well. Will see if I can fix.

@asturur
Copy link
Member

asturur commented Jul 6, 2018

i understand z-index, i use it often in web development.

The point is that there are no layers in fabricJS ( Groups would behave as layers in some condition and would not have this problem )

the zindex will basically break moveTo, sendBack, moveForward, bringToFront moveBackward.

Is unclear how it would work inside groups, where the array position is still dominant.

Is unclear how you set object E between object F and G if they have the same ZIndex.

It adds a partial feature and break others, or maybe just make them harder to understand.

In the case of default z-index parsed to 0 as your example code, is not possible to put an object in the middle of the stack without giving everyone a z-index or using moveTo, but moveTo then would not behave the same way when everyone has a z-index.

Not all application needs are part of the fabricJS core library, not should be because they increase the complexity of mantaining it working.

As i said, this can be a good extra functionality available in the official github.com/fabricjs organization, as are the guidelines and snap to object functions or an alignment tool.

@asturur
Copy link
Member

asturur commented Jul 6, 2018

Yes but please organize it as an override of standard methods rather than a modification of the core source

@maxhudson
Copy link
Author

@asturur groups seem tricky - yes. zIndex should probably be relative inside a group.

As far as middle of the stack, you can use negative zIndex values (So if you have a bunch of objects that are arbitrarily ordered, they can default to 0, and then you can put other objects behind with negative z or in front with positive z).

Makes sense about not over-adding to the core feature set unnecessarily. Every time I've used fabric in the past I've had difficulty getting things to display in the right order and had trouble with related bugs the application that uses fabric gets features added, but I understand prioritizing stability/simplicity/not-changing-api over working on a better solution. The fact that you support bring/send/moveTo indicates that a lot of people do use them, but from what I can tell they are not the best solution to an already difficult problem.

Perhaps I'll write a plugin if this ends up getting closed.

@stale
Copy link

stale bot commented Jan 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jan 25, 2020
@stale stale bot closed this Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue marked as stale by the stale bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants