switch from GL_TRIANGLE_FAN to GL_TRIANGLE_STRIP#538
Merged
Conversation
This is a work-around for a bug in VmWare SVGA3D, which renders GL_TRIANGLE_FAN incorrectly. Triangle strips are used more often than triangle fans, so bugs with triangle fans are less likely to be tested and reported to driver vendors.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSwitches room quad rendering from GL_TRIANGLE_FAN to GL_TRIANGLE_STRIP and updates the legacy room tex/acolor vertex shader to generate vertices with the correct ordering and improved documentation/comments about triangle strip vs fan winding. Sequence diagram for room quad rendering with GL_TRIANGLE_STRIPsequenceDiagram
participant App as App
participant LegacySimpleMesh as LegacySimpleMesh
participant Functions as Functions_gl
participant VertexShader as VertexShader_main
App->>LegacySimpleMesh: drawRoomQuad(gl, numInstances)
LegacySimpleMesh->>Functions: glDrawArraysInstanced(GL_TRIANGLE_STRIP, 0, 4, numInstances)
loop for each instance
Functions->>VertexShader: main(gl_VertexID)
VertexShader->>VertexShader: ioffset = ioffsets[gl_VertexID]
VertexShader-->>Functions: position, texCoord
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider removing the
#if 0block and the unused GL_TRIANGLE_FAN code from the shader to avoid carrying dead code and reduce maintenance overhead; a brief comment on why strips are used should be sufficient. - Since the triangle strip winding alternates, it may be helpful to explicitly note in a short comment whether any downstream logic assumes consistent front-face orientation, so future changes don’t accidentally rely on the old fan behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider removing the `#if 0` block and the unused GL_TRIANGLE_FAN code from the shader to avoid carrying dead code and reduce maintenance overhead; a brief comment on why strips are used should be sufficient.
- Since the triangle strip winding alternates, it may be helpful to explicitly note in a short comment whether any downstream logic assumes consistent front-face orientation, so future changes don’t accidentally rely on the old fan behavior.
## Individual Comments
### Comment 1
<location path="src/resources/shaders/legacy/room/tex/acolor/vert.glsl" line_range="28-29" />
<code_context>
+ // GL_TRIANGLE_STRIP
+ // 2--3 Note: Triangle strips alternate CCW/CW winding on every other triangle. This means...
+ // |\ | triangle 012 is CCW order, but triangle 123 is CW order (backwards).
+ // | \| Keep in mind that OpenGL does not actually draw the triangle backwards,
+ // 0--1 so it does not affect glFrontFace() or the gl_FrontFacing variable.
+ const ivec3[4] ioffsets = ivec3[4](ivec3(0, 0, 0), ivec3(1, 0, 0), ivec3(0, 1, 0), ivec3(1, 1, 0)); // strip
+#endif
</code_context>
<issue_to_address>
**issue:** Clarify the impact of alternating winding on glFrontFace/gl_FrontFacing.
The note about triangle strips not affecting `glFrontFace()` / `gl_FrontFacing` is inaccurate. Alternating winding in strips *does* change which triangles are classified as front-facing/back-facing under the current `glFrontFace` setting, which in turn impacts `gl_FrontFacing` and face culling. Please reword this to avoid misleading anyone relying on those behaviors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+28
to
+29
| // | \| Keep in mind that OpenGL does not actually draw the triangle backwards, | ||
| // 0--1 so it does not affect glFrontFace() or the gl_FrontFacing variable. |
There was a problem hiding this comment.
issue: Clarify the impact of alternating winding on glFrontFace/gl_FrontFacing.
The note about triangle strips not affecting glFrontFace() / gl_FrontFacing is inaccurate. Alternating winding in strips does change which triangles are classified as front-facing/back-facing under the current glFrontFace setting, which in turn impacts gl_FrontFacing and face culling. Please reword this to avoid misleading anyone relying on those behaviors.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #538 +/- ##
=======================================
Coverage 25.69% 25.69%
=======================================
Files 521 521
Lines 43187 43187
Branches 4708 4708
=======================================
Hits 11099 11099
Misses 32088 32088 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a work-around for a bug in VmWare SVGA3D,
which renders GL_TRIANGLE_FAN incorrectly.
Triangle strips are used more often than triangle fans, so bugs with triangle fans are less likely to be tested and reported to driver vendors.
Summary by Sourcery
Switch room quad rendering from GL_TRIANGLE_FAN to GL_TRIANGLE_STRIP to work around a VMware SVGA3D bug and adjust the vertex shader accordingly.
Enhancements: