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
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 204 additions & 6 deletions js/isomer.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ function Isomer(canvasId, options) {
*/
this.colorDifference = 0.20;
this.lightColor = options.lightColor || new Color(255, 255, 255);

/**
* 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

}

/**
Expand Down Expand Up @@ -67,25 +72,218 @@ Isomer.prototype._translatePoint = function (point) {
/**
* Adds a shape or path to the scene
*
* This method also accepts arrays
* This method also accepts arrays.
* By default or if expertMode=0, shapes are drawn
*/
Isomer.prototype.add = function (item, baseColor) {
Isomer.prototype.add = function (item, baseColor, expertMode, name) {
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of another parameter i'd like to have a bunch of add calls, which maybe just add them to the scene. A final render method will order and draw them, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

This expertMode parameter was for backward compatibility,
but if it's not needed we can remove it and do the add-add-render instead.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah let's run with that :)

Copy link
Owner

Choose a reason for hiding this comment

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

If you're super hesitant feel free to copypasta this add method and I can make the transition.

Copy link
Author

Choose a reason for hiding this comment

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

Do we keep the "name" parameter, which is to adress #34 ? (useful to remove/modify an element of the scene during an animation)

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 !

if (Object.prototype.toString.call(item) == '[object Array]') {
for (var i = 0; i < item.length; i++) {
this.add(item[i], baseColor);
this.add(item[i], baseColor, expertModeValid, name);
}
} else if (item instanceof Path) {
this._addPath(item, baseColor);
if(expertModeValid == 1){
this.paths[this.paths.length] = {path:item, color:baseColor, shapeName:(name||'')};
} else {
this._addPath(item, baseColor);
}
} else if (item instanceof Shape) {
/* Fetch paths ordered by distance to prevent overlaps */
var paths = item.orderedPaths();
for (var i in paths) {
this._addPath(paths[i], baseColor);
if(expertModeValid == 1){
this.paths[this.paths.length] = {path:paths[i], color:baseColor, shapeName:(name||'')};
} else {
this._addPath(paths[i], baseColor);
}
}
}
};

/**
* Draws the content of this.paths
* By default, does not sort the paths between shapes
*/
Isomer.prototype.draw = function(sortPath){
var sortValid = (typeof sortPath === 'number') ? sortPath : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Why isn't sortPath a boolean?

Copy link
Author

Choose a reason for hiding this comment

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

ok !

if(sortValid == 1){
this.sortPaths();
}
for (var i in this.paths){
this._addPath(this.paths[i].path, this.paths[i].color);
}
}


/**
* Sorts the paths contained in this.paths,
* ordered so that distant faces are displayed first
* */
Isomer.prototype.sortPaths = function () {
var Point = Isomer.Point;
var observer = new Point(-10, -10, 20);
var pathList = [];
for (var i = 0; i < this.paths.length; i++) {
var currentPath = this.paths[i];
pathList[i] = {
path: currentPath.path,
polygon: currentPath.path.points.map(this._translatePoint.bind(this)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create a new function.
Should be:

polygon: currentPath.path.points.map(this._translatePoint, this),

Copy link
Author

Choose a reason for hiding this comment

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

I took it from already existing _addPath, is there a difference between the two use cases ?

color: currentPath.color,
drawn: 0,
shapeName: currentPath.shapeName

};
}
this.paths.length = 0;

// topological sort

var drawBefore = [];
for (var i = 0 ; i < pathList.length ; i++){
drawBefore[i] = [];
}
for (var i = 0 ; i < pathList.length ; i++){
for (var j = 0 ; j < i ; j++){
if(this._hasIntersection(pathList[i].polygon, pathList[j].polygon)){
Copy link
Owner

Choose a reason for hiding this comment

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

A couple nits here as far as consistent spacing! I think we use two spaces in this library, but here we have 4.

Copy link
Author

Choose a reason for hiding this comment

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

oups

var cmpPath = pathList[i].path.closerThan(pathList[j].path, observer);
if(cmpPath < 0){
drawBefore[i][drawBefore[i].length] = j;
}
if(cmpPath > 0){
drawBefore[j][drawBefore[j].length] = i;
}
}
}
}
var drawThisTurn = 1;
var index = 0;
while(drawThisTurn == 1){
index++;
drawThisTurn = 0;
for (var i = 0 ; i < pathList.length ; i++){
if(pathList[i].drawn == 0){
var canDraw = 1;
for (var j = 0 ; j < drawBefore[i].length ; j++){
if(pathList[drawBefore[i][j]].drawn == 0){canDraw = 0;}
}
if(canDraw == 1){
this.add(pathList[i].path, pathList[i].color, 1, pathList[i].shapeName);
drawThisTurn = 1;
pathList[i].drawn = 1;
}
}
}
}
//purge
//could be done more in a smarter order, that's why drawn is is an element of pathList[] and not a separate array
for (var i = 0 ; i < pathList.length ; i++){
if(pathList[i].drawn == 0){
this.add(pathList[i].path, pathList[i].color, 1, pathList[i].shapeName);
}
}
};


//+ Jonas Raoni Soares Silva
//@ http://jsfromhell.com/math/is-point-in-poly [rev. #0]
//see also http://jsperf.com/ispointinpath-boundary-test-speed/2
function isPointInPoly(poly, pt){
for(var c = false, i = -1, l = poly.length, j = l - 1; ++i < l; j = i)
((poly[i].y <= pt.y && pt.y < poly[j].y) || (poly[j].y <= pt.y && pt.y < poly[i].y))
&& (pt.x < (poly[j].x - poly[i].x) * (pt.y - poly[i].y) / (poly[j].y - poly[i].y) + poly[i].x)
&& (c = !c);
return c;
}


/**
* Does polygonA has intersection with polygonB ?
* Naive approach done first : approximate the polygons with a rectangle
* Then more complex method : see if edges cross, or one contained in the other
*/
Isomer.prototype._hasIntersection = function(pointsA, pointsB) {
var i, j;
//console.log("_hasIntersection");
var AminX = pointsA[0].x;
var AminY = pointsA[0].y;
var AmaxX = AminX;
var AmaxY = AminY;
var BminX = pointsB[0].x;
var BminY = pointsB[0].y;
var BmaxX = BminX;
var BmaxY = BminY;
for(i = 0 ; i < pointsA.length ; i++){
AminX = Math.min(AminX, pointsA[i].x);
AminY = Math.min(AminY, pointsA[i].y);
AmaxX = Math.max(AmaxX, pointsA[i].x);
AmaxY = Math.max(AmaxY, pointsA[i].y);
}
for(i = 0 ; i < pointsB.length ; i++){
BminX = Math.min(BminX, pointsB[i].x);
BminY = Math.min(BminY, pointsB[i].y);
BmaxX = Math.max(BmaxX, pointsB[i].x);
BmaxY = Math.max(BmaxY, pointsB[i].y);
Copy link
Owner

Choose a reason for hiding this comment

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

Can we maybe clean up these lines? A big wall of vars is admittedly very intimidating.

var AmaxX = Math.max.apply(null, pointsA.map(function(p) { return p.x })).

I think I'd prefer the naming convention: AXmax, AXmin, BYmin, etc

Copy link
Author

Choose a reason for hiding this comment

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

Didn't know this syntax, obviously better for clarity

Choose a reason for hiding this comment

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

If you're going to be mapping over arrays and getting properties out, maybe prop would be a useful addition?

}

if(((AminX <= BminX && BminX <= AmaxX) || (BminX <= AminX && AminX <= BmaxX)) &&
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a comment explaining this big conditional?

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will.
For the record, this big conditional does the following :
For each of the 2 polygons, we consider the smallest rectangle that contains it. If the 2 rectangles don't intersect, then the 2 polygons don't intersect, so we can early abort the computation.

Copy link
Owner

Choose a reason for hiding this comment

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

So it won't cover every case? I can imagine two triangles whose bounding boxes intersect, but they themselves do not intersect. How would that case be handled? Something to worry about?

Copy link
Author

Choose a reason for hiding this comment

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

It covers every case, I was not clear enough. Let me explain in another way.
The hasIntersection function returns true if the 2 given polygons intersect.
It works this way :

IF the bounding boxes of the polygons do not intersect, return false (this is very fast to compute, and it is a common case)

ELSE check if the edge of one polygon crosses an edge of the other, or if one polygon is inside the other (this takes more time to compute).

So the point is that we do this bounding boxes thing to avoid doing the complex part every time.

Copy link
Owner

Choose a reason for hiding this comment

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

A-ha, makes sense! Maybe leave a comment to that effect?

((AminY <= BminY && BminY <= AmaxY) || (BminY <= AminY && AminY <= BmaxY))) {
// now let's be more specific
var polyA = pointsA.slice();
var polyB = pointsB.slice();
polyA.push(pointsA[0]);
polyB.push(pointsB[0]);

// see if edges cross, or one contained in the other
var deltaAX = [];
var deltaAY = [];
var deltaBX = [];
var deltaBY = [];
var rA = [];
var rB = [];
for(i = 0 ; i <= polyA.length - 2 ; i++){
deltaAX[i] = polyA[i+1].x - polyA[i].x;
deltaAY[i] = polyA[i+1].y - polyA[i].y;
//equation written as deltaY.x - deltaX.y + r = 0
rA[i] = deltaAX[i] * polyA[i].y - deltaAY[i] * polyA[i].x;
}
for(i = 0 ; i <= polyB.length - 2 ; i++){
deltaBX[i] = polyB[i+1].x - polyB[i].x;
deltaBY[i] = polyB[i+1].y - polyB[i].y;
rB[i] = deltaBX[i] * polyB[i].y - deltaBY[i] * polyB[i].x;
}


crossExam:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is useless.

Copy link
Author

Choose a reason for hiding this comment

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

ok I remove it, it was useful before I changed that block

for(i = 0 ; i <= polyA.length - 2 ; i++){
for(j = 0 ; j <= polyB.length - 2 ; j++){
if(deltaAX[i] * deltaBY[j] != deltaAY[i] * deltaBX[j]){
//case when vectors are colinear, or one polygon included in the other, is covered after
//two segments cross each other if and only if the points of the first are on each side of the line defined by the second and vice-versa
if((deltaAY[i] * polyB[j].x - deltaAX[i] * polyB[j].y + rA[i]) * (deltaAY[i] * polyB[j+1].x - deltaAX[i] * polyB[j+1].y + rA[i]) < -0.000000001 &&
Copy link
Owner

Choose a reason for hiding this comment

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

These equations are inherently difficult to reason about, so at first glance it might be useful to either A) have a TON of comments or B) separating these lines into multiple statements, maybe some temporary variables?

Copy link
Author

Choose a reason for hiding this comment

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

I need more than a minute to answer this ;) but you are right it lacks comments
(the good thing for me is that, with those 2 lines of comments I wrote one year ago, I understand what this code does :) )

(deltaBY[j] * polyA[i].x - deltaBX[j] * polyA[i].y + rB[j]) * (deltaBY[j] * polyA[i+1].x - deltaBX[j] * polyA[i+1].y + rB[j]) < -0.000000001){
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should return a boolean, not a number.

Copy link
Author

Choose a reason for hiding this comment

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

I guess I write too much C ;)
Thank you for your comments !

}
}
}
}

for(i = 0 ; i <= polyA.length - 2 ; i++){
if(isPointInPoly(polyB, {x:polyA[i].x, y:polyA[i].y})){
return 1;
}
}
for(i = 0 ; i <= polyB.length - 2 ; i++){
if(isPointInPoly(polyA, {x:polyB[i].x, y:polyB[i].y})){
return 1;
}
}

return 0;
} else {
return 0;
}

};

/**
* Adds a path to the scene
*/
Expand Down
47 changes: 47 additions & 0 deletions js/path.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,53 @@ Path.prototype.depth = function () {
return total / (this.points.length || 1);
};

/**
* If pathB ("this") is closer from the observer than pathA, it must be drawn after.
* It is closer if one of its vertices and the observer are on the same side of the plane defined by pathA.
*/
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!

}

Path.prototype._countCloserThan = function(pathA, observer) {
var Vector = Isomer.Vector;
var i = 0;

// the plane containing pathA is defined by the three points A, B, C
var AB = Vector.fromTwoPoints(pathA.points[0], pathA.points[1]);
var AC = Vector.fromTwoPoints(pathA.points[0], pathA.points[2]);
var n = Vector.crossProduct(AB, AC);

var OA = Vector.fromTwoPoints(Point.ORIGIN, pathA.points[0]);
var OU = Vector.fromTwoPoints(Point.ORIGIN, observer); //U = user = observer

// Plane defined by pathA such as ax + by + zc = d
// Here d = nx*x + ny*y + nz*z = n.OA
var d = Vector.dotProduct(n, OA);
var observerPosition = Vector.dotProduct(n, OU) - d;
var result = 0;
var result0 = 0;
for (i = 0; i < this.points.length; i++) {
var OP = Vector.fromTwoPoints(Point.ORIGIN, this.points[i]);
var pPosition = Vector.dotProduct(n, OP) - d;
if(observerPosition * pPosition >= 0.000000001){ //careful with rounding approximations
result++;
}
if(observerPosition * pPosition >= -0.000000001 && observerPosition * pPosition < 0.000000001){
result0++;
}
}

if(result == 0){
return 0;
} else {
return ((result + result0) / this.points.length);
}
};




/**
* Some paths to play with
Expand Down