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

Make PShapeOpenGL.setFill(int,int) always update tessellated vertices #902

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Junology
Copy link
Contributor

@Junology Junology commented Jan 8, 2025

Fixes #900 (and #677 too).

PShapeOpenGL provides "Tessellation update mode" beginTessellation() and endTessellation().
The current implementation of PShapeOpenGL.setFill(int,int) does the following:

  • if it is in "Tessellation update mode" (root.tessUpdate == true), it updates the color associated with tessellated vertices;
  • otherwise, it updates the "raw" data InGeometry and set "tessellation required" flag with markForTessellation().

The problem is that once the shape is tessellated, the tessellation doesn't seem to be performed again even after markForTessellation().
Another concern is that the behavior described above is inconsistent with PShape.OpenGL.setFill(int) overload, which does not care about "Tessellation update mode" and updates tessellated vertices anyway.

This PR makes PShapeOpenGL.setFill(int,int) behaves like the other overloads.
As a result, it also ignores "Tessellation update mode".
I am not 100% sure it is safe, since beginTessellation() and endTessellation() look doing a lot of stuff.
In particular, I think this change should be tested for the cases where tessellation creates new vertices.
Since I do not know the tessellation process in detail, such test cases are really appreciated.

@hx2A
Copy link
Collaborator

hx2A commented Jan 8, 2025

Thank you, @Junology , for this PR!

Does setStroke() need a similar code change? We should probably review the set methods in this class to ensure that where appropriate there is some consistency between their implementations.

@Junology
Copy link
Contributor Author

Junology commented Jan 9, 2025

If this change makes sense, then yes, setStroke() also needs a similar change.
Thank you for pointing it out; I will make a commit on that later.
But, before anything, I'd like to make sure it is really safe; the tessellation process is too complicated to understand all the details.

Exploring git log, I found that the "tessellation update mode" beginTessUpdate/endTessUpdate (later renamed to the current ones) were introduced in the commit 6595427.
Does someone remember the logic behind the mode at that point?
In particular, what types of operations are supposed to be enclosed with those functions?
Is it really safe to update colors outside of the mode, and how about the other attributes, especially when tessellation creates some new vertices?

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.

PShape setFill(indx, color) not working in Processing 4
2 participants