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

Add PolyLineT scaled() and offsetted(). #1034

Merged
merged 1 commit into from Oct 21, 2016

Conversation

Projects
None yet
5 participants
@drewish
Contributor

drewish commented Jul 18, 2015

Following the pattern in RectT adding some methods to return copies rather than modifying in place.

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Jul 19, 2015

Collaborator

Seems nice to me to have the congruency. Are we all cool with the name offsetted()? I personally can't think of anything better that follows the existing patterns.

Collaborator

richardeakin commented Jul 19, 2015

Seems nice to me to have the congruency. Are we all cool with the name offsetted()? I personally can't think of anything better that follows the existing patterns.

@gaborpapp

This comment has been minimized.

Show comment
Hide comment
@gaborpapp

gaborpapp Jul 19, 2015

Contributor

I think RectT has getOffset() instead of offsetted(). So one of them should change for consistency.

Contributor

gaborpapp commented Jul 19, 2015

I think RectT has getOffset() instead of offsetted(). So one of them should change for consistency.

@drewish

This comment has been minimized.

Show comment
Hide comment
@drewish

drewish Jul 19, 2015

Contributor

I tried to follow the pattern as best I could but I'm not really a fan of that name. I'm open to what ever the consensus is.

I'm going to rebase it to pick up the reverse addition.

Contributor

drewish commented Jul 19, 2015

I tried to follow the pattern as best I could but I'm not really a fan of that name. I'm open to what ever the consensus is.

I'm going to rebase it to pick up the reverse addition.

Show outdated Hide outdated include/cinder/PolyLine.h
PolyLineT<T> scaled( const T &scaleFactor, T scaleCenter = T() ) const;
void offset( const T &offsetBy );
PolyLineT<T> offsetted( const T &offsetBy ) const;
void reverse();

This comment has been minimized.

@drewish

drewish Jul 19, 2015

Contributor

I guess GitHub uses 8 spaces per tab? I'm using 4 per the Contributing guidelines.

@drewish

drewish Jul 19, 2015

Contributor

I guess GitHub uses 8 spaces per tab? I'm using 4 per the Contributing guidelines.

@drewish

This comment has been minimized.

Show comment
Hide comment
@drewish

drewish Jul 19, 2015

Contributor

Oh speaking of the Rect class... I'd love to follow it's pattern and add transform() and transformed() methods.

Contributor

drewish commented Jul 19, 2015

Oh speaking of the Rect class... I'd love to follow it's pattern and add transform() and transformed() methods.

@drewish

This comment has been minimized.

Show comment
Hide comment
@drewish

drewish Jul 19, 2015

Contributor

Ah so looks like transform/transformed will both run into the same precision issues were touched on in #1007. The suggestion there was a freestanding function and I think that might work well for this. It would avoid a lot of issues with trying to get the right precision and dimensions (I believe you'd want to turn vec2 into vec3 and vec3 into vec4 so you could use homogeneous coordinates).

Contributor

drewish commented Jul 19, 2015

Ah so looks like transform/transformed will both run into the same precision issues were touched on in #1007. The suggestion there was a freestanding function and I think that might work well for this. It would avoid a lot of issues with trying to get the right precision and dimensions (I believe you'd want to turn vec2 into vec3 and vec3 into vec4 so you could use homogeneous coordinates).

@drewish

This comment has been minimized.

Show comment
Hide comment
@drewish

drewish Sep 21, 2015

Contributor

Any interest in this?

Contributor

drewish commented Sep 21, 2015

Any interest in this?

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Sep 24, 2015

Collaborator

I think there is, and thanks for contributing this work, however we're sort of in a focus on getting through changes necessary for 0.9.0 release. These are pure additions, so perhaps we should wait to sift through the api choices until 0.9.1-dev begins.

Collaborator

richardeakin commented Sep 24, 2015

I think there is, and thanks for contributing this work, however we're sort of in a focus on getting through changes necessary for 0.9.0 release. These are pure additions, so perhaps we should wait to sift through the api choices until 0.9.1-dev begins.

@paulhoux paulhoux added the feature label Dec 17, 2015

@drewish

This comment has been minimized.

Show comment
Hide comment
@drewish

drewish Apr 2, 2016

Contributor

@richardeakin seems like stuff has settled down, mind giving it another look now?

Contributor

drewish commented Apr 2, 2016

@richardeakin seems like stuff has settled down, mind giving it another look now?

@richardeakin

This comment has been minimized.

Show comment
Hide comment
@richardeakin

richardeakin Apr 3, 2016

Collaborator

We'll try to get these looked at very soon, along with your other PolyLine improvements. Personally I'm wrapping up a 3 month project that has been consuming my time for the last few weeks. Thanks again for these contributions..

Collaborator

richardeakin commented Apr 3, 2016

We'll try to get these looked at very soon, along with your other PolyLine improvements. Personally I'm wrapping up a 3 month project that has been consuming my time for the last few weeks. Thanks again for these contributions..

@andrewfb

This comment has been minimized.

Show comment
Hide comment
@andrewfb

andrewfb Oct 18, 2016

Collaborator

Just to be frank, the naming between Rect, Area, PolyLine and Shape2d / Path2d are all in serious need of cleanup and unification, particularly in light of our move to GLM. I'm up for merging this, acknowledging that we're due for a serious, breaking refactor in the near future.

Collaborator

andrewfb commented Oct 18, 2016

Just to be frank, the naming between Rect, Area, PolyLine and Shape2d / Path2d are all in serious need of cleanup and unification, particularly in light of our move to GLM. I'm up for merging this, acknowledging that we're due for a serious, breaking refactor in the near future.

@andrewfb andrewfb merged commit 75e33f8 into cinder:master Oct 21, 2016

@drewish drewish deleted the drewish:polyline-more-ed branch Oct 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment