-
Notifications
You must be signed in to change notification settings - Fork 3
Fix getAttribLocation to return -1 for built-in attributes per WebGL 2.0 spec #412
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
base: main
Are you sure you want to change the base?
Conversation
- Add check for built-in attribute names (gl_VertexID, gl_InstanceID, gl_Position, gl_PointSize) - Return -1 immediately for built-in attributes per WebGL 2.0 spec §5.18 - Add <set> include for std::set container - Complies with WebGL 2.0 specification requirement Co-authored-by: EndlessJour9527 <[email protected]>
| // WebGL 2.0 spec §5.18: Built-in attributes always return -1 | ||
| static const std::set<string> kBuiltInAttribs = { | ||
| "gl_VertexID", "gl_InstanceID", "gl_Position", "gl_PointSize"}; | ||
| if (kBuiltInAttribs.count(name)) | ||
| { | ||
| info.GetReturnValue().Set(Number::New(isolate, -1)); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please impl in client/graphics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Moved the implementation to client/graphics/webgl_context.cpp in commit c135954. The built-in attribute check now happens at the graphics layer in WebGLContext::getAttribLocation(), which returns nullopt for built-in attributes. The script bindings layer correctly converts this to -1.
- Moved implementation from script bindings to WebGLContext::getAttribLocation - Returns nullopt for built-in attributes (gl_VertexID, gl_InstanceID, gl_Position, gl_PointSize) - Script bindings layer now correctly converts nullopt to -1 - Better architecture: centralizes logic at the graphics layer Addresses review feedback from @EndlessJour9527 Co-authored-by: EndlessJour9527 <[email protected]>
gl.getAttribLocation()was returning0for built-in attributes (gl_VertexID,gl_InstanceID,gl_Position,gl_PointSize) instead of the spec-mandated-1, causing Khronos conformance test failures.Changes
WebGLContext::getAttribLocation()nulloptfor built-in attributes (converted to-1by script bindings layer)<set>includeArchitecture
Implementation is in the graphics layer (
client/graphics/webgl_context.cpp) rather than script bindings layer, providing better separation of concerns and centralizing WebGL spec compliance logic at the appropriate architectural level.Spec Reference
WebGL 2.0 §5.18: "Applications may query the location of a built-in attribute by passing its name to getAttribLocation, but this will always return -1."
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.