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.

Sign up for GitHub

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

Jump to bottom

Add Ellipse interior/outline image support #211

Merged
merged 8 commits into from
Feb 1, 2018
Merged

Add Ellipse interior/outline image support #211

merged 8 commits into from
Feb 1, 2018

Conversation

zglueck
Copy link
Contributor

@zglueck zglueck commented Jan 31, 2018

Description of the Change

This change implements interior/outline image support. Implementation borrows heavily from Path/Polygon.

Why Should This Be In Core?

Unifies interior/outline API with Path/Polygon and provides additional capability for the Ellipse shape.

Applicable Issues

Closes #210
Compliments #18

@zglueck zglueck self-assigned this Jan 31, 2018
@zglueck zglueck requested a review from pdavidc January 31, 2018 19:32
@ghost ghost added the needs review label Jan 31, 2018
@zglueck
Copy link
Contributor Author

zglueck commented Jan 31, 2018

Closing this while I add texture coordinate matrix utility to AbstractShape.

@zglueck zglueck closed this Jan 31, 2018
@ghost ghost removed the needs review label Jan 31, 2018
@zglueck
Copy link
Contributor Author

zglueck commented Feb 1, 2018

I've updated this pull request to include the refactored computeTexCoordMatrix method added to AbstractShape and shared by Ellipse, Path, and Polygon.

@zglueck zglueck reopened this Feb 1, 2018
@ghost ghost added the needs review label Feb 1, 2018
@pdavidc
Copy link
Contributor

pdavidc commented Feb 1, 2018

This looks good. One note: there's a method naming convention for computing transforms:

computeTexCoordTransform
computeViewingTransform

For the general utility method in AbstractShape, how about computeRepeatingTexCoordTransform?

@zglueck
Copy link
Contributor Author

zglueck commented Feb 1, 2018

I missed the convention, sorry. What about decoupling "repeating" from the texture transform? Would a "screen constant" or "stipple" mention be better?

Potential Options:

  • computeScreenConstantTexCoordTransform
  • computeStippleTexCoordTransform
  • computeScreenScaledTexCoordTransform

What do you think?

@pdavidc
Copy link
Contributor

pdavidc commented Feb 1, 2018

To me, those names indicate how the method works. The best names indicate what the subject is trying to accomplish.

@zglueck
Copy link
Contributor Author

zglueck commented Feb 1, 2018

Ok, I've updated the name and pushed 👍

@zglueck zglueck merged commit 90cdd05 into develop Feb 1, 2018
@ghost ghost removed the needs review label Feb 1, 2018
@zglueck zglueck deleted the feature/#210-ellipse-texture branch February 1, 2018 20:36
@zglueck zglueck added this to the WWA v0.8.0 milestone Feb 1, 2018
pdavidc pushed a commit that referenced this pull request Feb 2, 2018
@zglueck @pdavidc
Add Ellipse interior/outline image support (#211)
aa31fd4 
* Add texture capabilities

* Add Ellipse texture fill to Shapes Dash and Fill tutorial

* Refactor disparate texture coordinate matrix computation to AbstractShape
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdavidc pdavidc pdavidc approved these changes

Assignees

@zglueck zglueck

Labels
None yet
Milestone
WWA v0.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@zglueck @pdavidc