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

Closed
wants to merge 1 commit into 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?

@Junology
Copy link
Contributor Author

I realized the change done in this PR is UNSAFE.
Indeed, the modified version of setFill(int,int) can cause ArrayIndexOutOfBoundsException if it is used in "tessellation update mode".

Here is the code I tested:

PShape[] t;

void setup() {
  size(300, 200, P2D);
  t = new PShape[2];
  for (int i = 0; i < t.length; ++i) {
    t[i] = createShape();
    t[i].beginShape();
    t[i].fill(#FFFFFFFF);
    t[i].vertex(0, 0);
    t[i].vertex(0, 100);
    t[i].vertex(100, 0);
    t[i].vertex(100, 100);
    t[i].endShape(CLOSE);
  }
}

void draw() {
  background(0);
  float offset = (float) (width - 100 * t.length) / (t.length + 1);
  for (int i = 0; i < t.length; ++i) {
    shape(t[i], offset + (100 + offset) * i, 50);
  }
}

void mousePressed() {
  // Reset fill colors in different ways
  t[0].setFill(#FFFF0000);

  // setFill(int,int) in "tessellation update mode" can cause ArrayIndexOutOfBoundsException
  // t[1].beginTessellation();
  for (int i = 0; i < t[1].getVertexCount(); i++) {
    t[1].setFill(i, #FFFF0000);
  }
  // t[1].endTessellation();
}

First notice that the above program does not work as expected; the color of the shape t[1] changes only partially though the for-loop seems traversing all the vertices.
Second, uncommenting the lines of beginTessellation()/endTessellation() causes ArrayIndexOutOfBoundsException.

The source of the exception lies in how the function getVertexCount() behaves; it usually counts the vertices in the raw shape (i.e. inGeo), but in "tessellation update mode", it looks at the tessellated shape tessGeo.
Hence, if there are new vertices created in tessellation, the returned value can exceed the size of the vertex array of inGeo, and that is why the exception.

I decided to close the PR, thinking it is difficult to fix the problem mentioned above by refactoring setFill(int,int) itself; for a vertex in the raw shape given, there is no way to determine the corresponding vertex in the tessellated shape.

If one is sure what happens in tessellation, then one can use the function in "tessellation update mode" anyway.
On the other hand, if one wants to use it without knowing tessellation, then fixing #900 is still necessary.
Making markForTessellation() really cause re-tessellation seems to be the only way to go.

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