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

CurvePath.getPoints() for Arcs resolution should be doubled #9168

Merged
merged 2 commits into from Jul 1, 2016

Conversation

@zz85
Copy link
Contributor

zz85 commented Jun 18, 2016

  • like previous behaviour
  • see #9079
@zz85 zz85 mentioned this pull request Jun 18, 2016
0 of 4 tasks complete
@zz85

This comment has been minimized.

Copy link
Contributor Author

zz85 commented Jun 18, 2016

🤔 hmm why is there conflicts?

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Jun 20, 2016

Not sure. Do you mind pulling from dev and resolving the conflicts?

@zz85 zz85 force-pushed the zz85:shapes_resolution branch from ce7d74f to 707b3ea Jun 20, 2016
@zz85

This comment has been minimized.

Copy link
Contributor Author

zz85 commented Jun 20, 2016

Green🚥 now :D

@zz85 zz85 force-pushed the zz85:shapes_resolution branch from 707b3ea to 094835b Jun 20, 2016
@zz85 zz85 force-pushed the zz85:shapes_resolution branch from 094835b to 6f9567a Jun 20, 2016
@rfm1201

This comment has been minimized.

Copy link
Contributor

rfm1201 commented Jun 20, 2016

If you want to have the same behavior, you also need to change for splineThru because previously there was a var n = divisions * args[ 0 ].length; before the loop.
But I would prefer the caller to have more control on the step used internally in getPoints. In other words, me to choose, it is the example page page that I would update not CurvePath.
If you go back to the previous way, I think that the documentation should be clear on the real usage of the parameter. The only documentation related to getPoints is currently in Curve. There is no explanation on the parameter meaning.
I couldn't found it today, but a previous documentation (of Path or CurvePath, I don't remember) stated that the parameter was used for each part of the path. It was wrong, the magical internal multiplication factor was not reveled, but told a part of the truth.
The problem is that other use getPoints. Shape.extractPoints ( divisions ) documentation for example says,

divisions -- The number of divisions to create on the shape

When I read this, I don't understand what the code is going to produce. Maybe I'm the only one, but when I started using Path, the number of points returned by some functions (getPoints based one) was a mystery to me.

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Jun 20, 2016

I think I'll wait until you guys agree on this one 😁

@zz85

This comment has been minimized.

Copy link
Contributor Author

zz85 commented Jun 22, 2016

If you want to have the same behavior, you also need to change for splineThru

Good point, I should add that too.

But I would prefer the caller to have more control on the step used internally in getPoints

I was thinking of that, but I'm not sure if we exposing that would make getPoints() more difficult to use or not. For example, their configuration might require passing in a map of types eg.
[ THREE.LineCurve2, 2], [THREE.ArcCurve, divisions *2] etc...

I think I would be in favour of some sensible defaults. Perhaps the documentation could then be reflected along these lines to state it is not absolute number of divisions for curvePath.getPoints()

divisions -- units for the average number of division created for each curve segments.

@killroy42

This comment has been minimized.

Copy link

killroy42 commented Jun 29, 2016

I've made a small algorithm to use equally spaced points but still respect the "corners", by forcing points at the joints between curves. Currently it's just:

function getSpacedPointsWithCorners(curvePath, divisions) 

Example of the algorithm in my extrusion system using this shape:

shape.moveTo( 0,  20);
shape.bezierCurveTo(  5,  15,  15,  16,  20,  16);
shape.bezierCurveTo( 20,   0,  15, -16,   0, -20);
shape.bezierCurveTo(-15, -16, -20,   0, -20,  16);
shape.bezierCurveTo(-15,  16,  -5,  15,   0,  20);

It forces at least one division per curve. The screenshot is for divisions 4-13 (row 1) and 14-24 (row 2).

shield-extrusions

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Jun 29, 2016

@killroy42 that looks nice 😊

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Jun 29, 2016

@zz85 is this PR ready to go?

@zz85

This comment has been minimized.

Copy link
Contributor Author

zz85 commented Jun 30, 2016

@mrdoob No harm merging this first. Always open for others to improve on this :)

@killroy42

This comment has been minimized.

Copy link

killroy42 commented Jun 30, 2016

Mine isn't ready yet. I'm finishing of the UV generation for the profile/outline extrusion, then I gotta @mrdoob -ify it (by adding lots of white space EVERYWHERE :P) and make sure it's PR-worthy, since it'll be my first one here, and I want it to shine like a beacon, yada-yada, etc-etc...

@killroy42

This comment has been minimized.

Copy link

killroy42 commented Jun 30, 2016

Sorry if I'm hijacking, but I managed to get the UVs working well enough, so only really need code cleanup and PR now.
extrusion-with-uvs

@mrdoob mrdoob merged commit 8dcd65e into mrdoob:dev Jul 1, 2016
@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Jul 1, 2016

Thanks!

@rfm1201 rfm1201 mentioned this pull request Jul 1, 2016
@rfm1201

This comment has been minimized.

Copy link
Contributor

rfm1201 commented Jul 1, 2016

@killroy42, I really like your getSpacedPointsWithCorners, and I think it may be more usefull than the current getSpacedPoints that often requires many points to have the rigth shape. But getSpacedPoints does what its name suggests, is easy to understand and must be kept (in my opinion).
Anyway, it is @zz85 that will tell you how he would like to add (or not) your suggestion.
If you need help for your first PR, you may look at https://github.com/mrdoob/three.js/wiki/How-to-contribute-to-three.js. The thing I didn't understand when I did my firs PR is that you have to fork three.js as first step.

@killroy42

This comment has been minimized.

Copy link

killroy42 commented Jul 1, 2016

Yeah, I'm familiar with the fork/PR process (done it on other projects), but I feel @mrdoob has some strict style requirements, lol, so I'm wary ;) It's not a complex algo, so shouldn't be too hard to clean up.
Just needs cleaning up and testing as well as integration into the curves example.

function getSpacedPointsWithCorners(curvePath, divisions) {
    var points = [];
    var curves = curvePath.curves;
    var curveCount = curves.length;
    var curveLengths = [];
    var totalLength = 0;
    var i, curveDivisions, curvePoints;
    for(i = 0; i < curveCount; i++) {
        curveLengths[i] = curves[i].getLength();
        totalLength += curveLengths[i];
    }
    var currentLength = 0;
    for(i = 0; i < curveCount; i++) {
        currentLength += curveLengths[i];
        curveDivisions = Math.round(currentLength * divisions / totalLength - points.length);
        curveDivisions = Math.max(1, curveDivisions);
        curvePoints = curves[i].getSpacedPoints(curveDivisions);
        if(i < curveCount-1) curvePoints.length--;
        points = points.concat(curvePoints);
    }
    // Trim last point if it is equal to first
    if(points[0].distanceToSquared(points[points.length-1]) < Number.EPSILON) points.length--;
    return points;
}

It'd probably try to use "getLengths()", maybe, and perhaps get rid of the concat to avoid allocations. Oh and the "distanceToSquared" at the end kinda "smartly" decides if it's a closed curve or an open one, and perhaps should be done with an explicit argument, or just consistently one way or another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.