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

Path and multiple shapes ordering (solves #9) #39

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

Mysterix
Copy link

@Mysterix Mysterix commented May 8, 2014

First, good news : it can't break anything, as I only added functions (for the moment), and changed none.
The function to use is addOrdered, you can see examples here http://rdelav.fr/isomer/isomer3.html (and the following numbers)
addOrdered takes as input as list of {shape:yourShape, color:yourColor}, and display all the path of all the shapes of the input and display them in the correct order, according to their relative positions.

The algorithm used is the following :
We put all the paths of every shape in a big array
For every couple of path, we considerer their projection on the canvas (which are two 2D-polygons).
If their intesection is non-empty [see note 1], we add the rule that the one which is in front [see note 2] must be drawn after the other one.
To match all those rules, we sort the path using a topological sort.

Note1 : to know if the intesection is not empty, we compute if one edge of the first cuts an edge of the other, or if a vertex of one is contained in the other
Note2 : to know which one is is front of the other between two intesecting paths, we look at the percentage of vertices of one which are one the same side of the plane defined by the other than the observer. (I will show a drawing for that)
Note3 : we need to check that the projection of the paths is intersecting, otherwise you have common situation where A is in front of B which is in front of C which is in front of A (I will show a drawing for that too)
Note 4 : other problems I didn't think of initially : rounding approximations, and the exact position of the observer.
Note 5 : I found so many special cases and exceptions during my tests (that work in this version) that there may be cases I didn't think of, sorry for that.

In the future, instead of a function addOrdered, we can also have two functions, one to add a shape, and one to flush and draw all the added shapes.

@jdan
Copy link
Owner

jdan commented May 10, 2014

I'd like to eventually replace Isomer.prototype.add with this. Doable? How big is the performance hit.

Thanks for all the hard work you've put into this, by the way. Reading your code is a lot of fun.

@Mysterix
Copy link
Author

We can definitely do that, performance is not a limitation anymore (except if we have a very high number of path, but there is still room for code optimization)
I see several options :

First option : we don't change how Isomer.prototype.add works, we just replace Shape.prototype.orderedPaths with a new version.
Problem : it sorts the paths of each shape (so corrects bugs), but doesn't sort shapes between them

Second option : each object Isomer contains an array (let's name it path_list)
When we call Isomer.prototype.add, instead of drawing the paths, it adds them to the array (with the format {path, color, (shapeName)})
To actually draw the shapes, the user calls Isomer.prototype.draw, which has a parameter to say if we want to reorder paths or not (ordering is not always wanted, see for example the flappy bird in the gallery).
So, the user adds, adds, adds, (...) and then draw.
Bonus : it corrects issue #34
Extra bonus : it corrects issue #15 ! we just add a function detectPath(Point) that goes through the array from right to left, calling the function isPointInPoly until the point is in the path, and we return the shapeName of this path (that's why I added ths property). Maybe it's more efficient to compute that for every pixel once for all, I will try...

Guess which option I would vote for 😜

@Mysterix
Copy link
Author

ok I implemented "option 2" from my previous post.
Don't merge it yet, I will test it this afternoon (it's just in case you want to have a look)

@Mysterix
Copy link
Author

ok it works.
As it is, users have to do add iso.draw() at the end of their previously written code, I don't really know how to achieve perfect backward-compatibility

@Mysterix
Copy link
Author

Following a suggestion by jdan, now it manages backward-compatibility.
User code that worked will still work.

If you want the new features, you can write it like :

var expertMode = 1;
iso.add(Shape.Prism(Point(0, 2, 0), 1, 2, 1), red, expertMode, 'redPrism');
iso.add(Shape.Prism(Point(2, 0, 0), 2, 1, 1), blue, expertMode, 'blueBox');
iso.add(Shape.Prism(Point(1, 1, 0), 4, 4, 2), green, expertMode, 'greenStuff');
iso.add((Shape.extrude(Path.Star(Point(0,8,0), 1, 2, 4).rotateZ(Point.ORIGIN, Math.PI/6))), blue, expertMode, 'blueStar');  
iso.add(Shape.Cylinder(new Point(9, 5, 0), 4, 30, 6), blue, expertMode, 'blueCylinder');

iso.draw(1);

expertMode = 1 means : when you add a shape, put it in iso.paths instead of drawing them
draw(0) means : draw the content of iso.paths (0 or default = without ordering, 1 = with ordering)

The option shapeName is for advanced users who want to manipulate paths in some way (see for example clickDetection, coming soon)

*/
Isomer.prototype.add = function (item, baseColor) {
Isomer.prototype.add = function (item, baseColor, expertMode, name) {
var expertModeValid = (typeof expertMode === 'number') ? expertMode : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why expertMode isn't a boolean?
IMHO, something like this is more suitable:

expertMode = !!expertMode;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok !

@jdan
Copy link
Owner

jdan commented May 12, 2014

Okay I think adding shapes & paths one-by-one and then calling "render" at the end would work. This way we could bucket drawing operations and maybe even optimize them (later down the road).

So let's change this new "addOrdered" to a simple "add" and then a "render" method will actually draw everything. How does that sound?

@Mysterix
Copy link
Author

That's what I implemented in the last commits (see my comment above, the one that begins with "Following a suggestion by jdan")

@mcintyre321
Copy link

With regards to doing hit detection, this approach sounds like it will take O(n) to do a hit test, while testing against a background lookup canvas will be O(1) (unless I've misunderstood something). Is that right?

@Mysterix
Copy link
Author

You're perfectly right, and there is a solution :
It depends on how often you have to hit test compared to how often you have to redraw the scene.
If you need to hit test many times, it's better to compute the value once for all pixels.

By the way, with expertMode, each time you need to refresh the canvas (for example with animations), you have to empty this.paths (this.paths.length = 0)

@hallas
Copy link

hallas commented May 13, 2014

canvas API will support hit regions at some point, might be better to use

@Mysterix
Copy link
Author

There is a benchmark here :
http://jsperf.com/ispointinpath-boundary-test-speed/4
where isPointInPath from canvas API performs slower than pointInPoly,
I found that surprising, but took the fastest

@jdan
Copy link
Owner

jdan commented Aug 31, 2014

I'm going to change Isomer to use Three.js as a backend, instead of attempting to write my own graphics hacks.

I really do appreciate the work you've put into this pull request, but unfortunately I can't proceed in this direction and attempt to tackle more graphics problems that have already been solved in very elegant ways.

@jdan jdan closed this Aug 31, 2014
@jdan
Copy link
Owner

jdan commented Mar 22, 2015

I'd like to revisit this if you don't mind! After some thought I've realized that bringing in Three.js as a requirement for proper shape ordering is pretty unacceptable. You've done some amazing work here, and I'd really like to see it make its way into master.

Any interest in addressing some comments? I'll leave them - but if you're totally done with this library then no worries.

@jdan jdan reopened this Mar 22, 2015
/**
* List of {path, color, shapeName} to draw
*/
this.paths = [];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we call this scene or some sort?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it's a better name

@Mysterix
Copy link
Author

Of course I don't mind,
I tried to answer quickly to your questions,
I will try to answer more completely soon, I need to read again the code to refresh my memory...
Much more testing than what I did will also be required...

*/
Path.prototype.closerThan = function(pathA, observer) {
var result = pathA._countCloserThan(this, observer) - this._countCloserThan(pathA, observer);
return result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is kinda messy in this function...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I've already address that above :) They'll fix!

@cryptoquick
Copy link
Contributor

I agree that there's definitely value in this work. I'm looking forward to seeing the solution to this problem. I have come across need for a solution this same problem before, and was never successful. It makes me happy that someone is able to finally nail this one down. 😸

@cryptoquick
Copy link
Contributor

I built a thing that suffers a great deal from the issue described in #9:

http://cutecity.meteor.com/edit/new
https://github.com/cutecity/cutecity/tree/depth-sorting

I tried using a build from @Mysterix's repo, and I'm using the expertMode argument in addition to the draw method. It seems to work well, but it doesn't detect for whether a shape should be overwritten.

That'd be something I'd like to explore, perhaps by keeping an object literal with a sort of domain-specific hash notation (such as shapetype-x-y-z) to serve as a property name that then stores array indices pointing to a spot in the scene array. That way, a quick hash could be computed using properties known about the shape, and the object literal could be checked before adding the shape to the scene array. This method would be O(1), so it would add very little overhead to the .add method, and would reduce overhead from drawing multiple things to the same place, overwriting each other.

Should there also be a method to reset the scene? I know it's as easy as writing iso.scene = [], but that's only currently; if there are more things that need to be reset, it'd be good to have a method.

@jdan
Copy link
Owner

jdan commented Mar 28, 2015

Feel free to branch off and offer up those changes in a separate diff :) Sounds like a good idea to me!

jordan scales

http://jordanscales.com

On Fri, Mar 27, 2015 at 7:17 PM, Hunter Trujillo [email protected]
wrote:

I built a thing that suffers a great deal from the issue described in #9:
http://cutecity.meteor.com/edit/new
https://github.com/cutecity/cutecity/tree/depth-sorting
I tried using a build from @Mysterix's repo, and I'm using the expertMode argument in addition to the draw method. It seems to work well, but it doesn't detect for whether a shape should be overwritten.
That'd be something I'd like to explore, perhaps by keeping an object literal with a sort of domain-specific hash notation (such as shapetype-x-y-z) to serve as a property name that then stores array indices pointing to a spot in the scene array. That way, a quick hash could be computed using properties known about the shape, and the object literal could be checked before adding the shape to the scene array. This method would be O(1), so it would add very little overhead to the .add method, and would reduce overhead from drawing multiple things to the same place, overwriting each other.

Should there also be a method to reset the scene? I know it's as easy as writing iso.scene = [], but that's only currently; if there are more things that need to be reset, it'd be good to have a method.

Reply to this email directly or view it on GitHub:
#39 (comment)

@whoeverest
Copy link
Collaborator

@Mysterix If you're into it, I'd be interested to try and solve #15 on your fork. I had a similar idea, to keep track of the objects in the scene, then detect hits using ctx.isPointInMath, but I see you've already done most of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants