Skip to content

Conversation

zz85
Copy link
Contributor

@zz85 zz85 commented Nov 26, 2015

  • Basic unit tests added for ClosedSplineCurve3 and CatmullRomCurve3 (inside test/unit/unittests_sources.html)
  • CatmullRomCurve3 supports .closed property
  • ClosedSplineCurve3 now has depreciation message
  • ClosedSplineCurve3 uses CatmullRomCurve3 internally for transition plan

Relates to #7587

If we are ok on the change, I could help to update the documentation too

@WestLangley @mrdoob

@zz85
Copy link
Contributor Author

zz85 commented Nov 26, 2015

btw @gero3 - nice stuff, I see ci.threejs also works on the tests :) http://ci.threejs.org/api/pullrequests/7686/test/unit/unittests_sources.html?filter=curve

just curious, where is ci hosted on? are you able to turn on gzip compression (would help speed up the loading ;)

@WestLangley
Copy link
Collaborator

Don't you need to create a .closed property, then?

How does .closed differ from .autoClose?

@zz85
Copy link
Contributor Author

zz85 commented Nov 28, 2015

.closed if undefined would default to being false. .closed when set to true would mean that its a catmullrom path which is in a loop.

I believe .autoClose is intended for Path which is to support the canvas like 2d api . closePath().

Where else this .closed means that since the catmullrom path is circular, the weights of the first points would use the last points and the last points would be affected by the first points.

@mrdoob
Copy link
Owner

mrdoob commented Nov 28, 2015

How about setting .close to false by default? That way we don't modify the hidden class.

@zz85
Copy link
Contributor Author

zz85 commented Nov 30, 2015

hmm, I didn't think of that, now updated .closed to false. (would be useful if there's some v8 debugging tools which warns when additional hidden classes are created).

@zz85
Copy link
Contributor Author

zz85 commented Nov 30, 2015

👌Updated documentation too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

THREE.ClosedSplineCurve3 has been deprecated. Please use THREE.CatmullRomCurve3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @WestLangley

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your welcome, but I think you mean deprecated, not depreciated. (2 instances)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh yes!!! fixed again..

@zz85
Copy link
Contributor Author

zz85 commented Dec 15, 2015

bump

Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this go in Curve.js instead?

THREE.Curve = function () {
    this.closed = false;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now it feels to me that .closed is a property of CatmullRomCurve3 rather than Curve. If .closed is a property of Curve then all curves should support using this property which we currently don't...

Copy link
Owner

Choose a reason for hiding this comment

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

I see. Yeah, that makes sense.

mrdoob added a commit that referenced this pull request Dec 17, 2015
CatmullRomCurve3 supports .closed property
@mrdoob mrdoob merged commit d05bd4f into mrdoob:dev Dec 17, 2015
@mrdoob
Copy link
Owner

mrdoob commented Dec 17, 2015

Thanks!

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.

3 participants