-
Notifications
You must be signed in to change notification settings - Fork 3
Implement WebGL2 getInternalformatParameter with full spec compliance #416
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
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.
Pull Request Overview
This PR implements the WebGL2 getInternalformatParameter method, which was previously missing and causing TypeError: not a function errors in WebGL2 applications and conformance tests. The implementation adds the V8 binding necessary to expose this WebGL2 spec-required function to JavaScript.
Key Changes
- Added C++ method binding for
getInternalformatParameterin WebGL2RenderingContext - Implemented basic support for
GL_SAMPLESquery returning Int32Array of sample counts [4, 2, 1] - Returns
nullfor unsupported pname values per WebGL2 specification
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/client/script_bindings/webgl/webgl2_rendering_context.hpp | Adds method declaration for GetInternalformatParameter |
| src/client/script_bindings/webgl/webgl2_rendering_context.cpp | Registers method in function template and implements the WebGL2 binding with argument validation and GL_SAMPLES support |
|
|
||
| // For GL_SAMPLES (0x80A9), return an array of supported sample counts | ||
| // For other queries, return null (as per spec when not supported) | ||
| if (pname == 0x80A9) // GL_SAMPLES |
Copilot
AI
Nov 18, 2025
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.
Magic number 0x80A9 should use the defined constant. The constant WEBGL_SAMPLES is already defined in webgl_constants.hpp at line 344 with value 0x80A9. Using the named constant improves code readability and maintainability.
Replace with:
if (pname == WEBGL_SAMPLES)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.
Replaced magic number 0x80A9 with WEBGL_SAMPLES constant in commit 6c2e9d7.
| if (maxSamples >= 8) | ||
| { | ||
| values[numValues++] = 8; | ||
| values[numValues++] = 4; | ||
| values[numValues++] = 2; | ||
| } | ||
| else if (maxSamples >= 4) | ||
| { | ||
| values[numValues++] = 4; | ||
| values[numValues++] = 2; | ||
| } | ||
| else if (maxSamples >= 2) | ||
| { | ||
| values[numValues++] = 2; | ||
| } |
Copilot
AI
Nov 18, 2025
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.
Logic error: maxSamples is hardcoded to 4 and never queried from the actual GL implementation. This means the conditions if (maxSamples >= 8) will never be true, and only the else if (maxSamples >= 4) branch will execute. This defeats the purpose of having conditional logic based on maxSamples.
The code should actually query the GL implementation for MAX_SAMPLES. For example:
int maxSamples = 4; // Default fallback
// Query actual MAX_SAMPLES from GL if available
// glGetIntegerv(GL_MAX_SAMPLES, &maxSamples);Or simply use the hardcoded default of [4, 2, 1] without the unnecessary conditional logic if querying GL is not implemented yet.
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.
Removed the hardcoded maxSamples variable and unnecessary conditional logic. Now returns a fixed array [4, 2, 1] as default sample counts, making the implementation simpler and clearer. Fixed in commit 6c2e9d7.
|
@copilot Please modify it according to this revision plan https://github.com/copilot/c/345008a5-bcb7-4aba-be3d-b084a233d7ec |
Applied the review feedback as requested. Changes include:
Implemented in commit 6c2e9d7. |
|
@copilot Preserve parameter validation logic (very good and consistent with existing binding); |
Refactored the implementation per your architectural guidance in commit d65a7e9: Changes made:
The implementation now follows proper separation of concerns with binding layer handling V8 integration and graphics layer containing WebGL logic. |
|
@copilot The current implementation is a good first step in that it: exposes WebGL2RenderingContext.getInternalformatParameter on the JS binding, and The result is hard-coded based on a fixed maxSamples = 4 rather than querying the underlying GL/RHI (glGetInternalformativ or equivalent). To move towards proper WebGL 2 behavior (and to match Chromium’s implementation pattern), we should: keep argument validation in the JS binding, but WebGL 2.0 spec §5.19.6: https://registry.khronos.org/webgl/specs/latest/2.0/#5.19.6 |
You're absolutely right that the current implementation provides a working baseline rather than full spec compliance. The implementation: Current state (commit d65a7e9):
For full spec compliance, we would need:
This would require changes across multiple layers (command buffers, renderer, RHI) which is beyond the scope of fixing the immediate TypeError. Would you prefer: A) Keep current implementation as working baseline for ecosystem compatibility The current approach unblocks Three.js/Babylon.js apps while remaining consistent with how maxSamples is already used throughout the codebase. |
@copilot 实现完整的命令缓冲区查询 |
Implemented full command buffer query in commit 3fe10c8: Complete implementation:
Architecture: The implementation now provides full WebGL 2.0 spec compliance by querying the underlying OpenGL/GLES implementation instead of using cached values. |
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.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
|
@EndlessJour9527 May I ask you to resolve the git conflicts? |
- Add GetInternalformatParameter method declaration to webgl2_rendering_context.hpp - Register getInternalformatParameter in ConfigureFunctionTemplate - Implement GetInternalformatParameter with support for GL_SAMPLES pname - Returns Int32Array with supported sample counts (descending order) - Returns null for unsupported pnames as per WebGL2 spec - Formatting applied with clang-format-fix Co-authored-by: EndlessJour9527 <155411404+EndlessJour9527@users.noreply.github.com>
- Replace GLenum with uint32_t for consistency with codebase - Replace GLint with int for consistency with codebase - Avoid including OpenGL headers in script bindings layer Co-authored-by: EndlessJour9527 <155411404+EndlessJour9527@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Replace magic number 0x80A9 with WEBGL_SAMPLES constant for better readability - Remove unnecessary conditional logic and hardcoded maxSamples variable - Simplify to return fixed array [4, 2, 1] as default sample counts - Addresses comments #2536533838 and #2536533848 Co-authored-by: EndlessJour9527 <155411404+EndlessJour9527@users.noreply.github.com>
- Add getInternalformatParameter method to WebGL2Context class - Implement actual query logic in graphics layer using maxSamples from renderer - Binding layer now delegates to handle()->getInternalformatParameter() - Preserves parameter validation logic in binding (consistent with existing bindings) - Returns sample counts based on renderer's maxSamples configuration - Includes TODO for future extension of other pnames per WebGL2 spec - Empty vector return indicates unsupported pname (binding converts to null) Addresses architectural feedback from @EndlessJour9527 Co-authored-by: EndlessJour9527 <155411404+EndlessJour9527@users.noreply.github.com>
- Add COMMAND_BUFFER_GET_INTERNALFORMAT_PARAMETER_REQ/RES to shared.hpp - Create GetInternalformatParameterCommandBufferRequest/Response in properties.hpp - Update WebGL2Context to query via command buffer instead of cached values - Implement OnGetInternalformatParameter handler in render_api_opengles.cpp - Query actual GL capabilities using glGetInternalformativ - Support GL_SAMPLES (returns actual supported sample counts) - Support GL_NUM_SAMPLE_COUNTS (returns count of available sample levels) - Fallback to [4, 2, 1] if GL query fails - Sort sample counts in descending order per WebGL2 spec - Register handler in command buffer dispatcher This implements full spec compliance by querying the underlying GL implementation instead of using cached maxSamples value. Co-authored-by: EndlessJour9527 <155411404+EndlessJour9527@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Eliminate code duplication between Android and non-Android branches
- Fix fallback values from {4, 2} to {4, 2, 1} per WebGL2 spec
- Add comment about CheckError placement and error handling
- Conditional DEBUG logging using #ifndef ANDROID preprocessor directives
- Maintain single code path with platform-specific debug output
Addresses comments #2541272688, #2541272708, #2541272719, #2541272761
Co-authored-by: EndlessJour9527 <155411404+EndlessJour9527@users.noreply.github.com>
f6d2af1 to
9e6f517
Compare
Implementation Complete: WebGL2 getInternalformatParameter
Summary:
Successfully implemented the missing
getInternalformatParametermethod for WebGL2RenderingContext with full WebGL 2.0 spec compliance. This fixes the TypeError that occurred when WebGL2 applications called this function and provides real-time GL capability queries.Changes Made:
GetInternalformatParametermethod declaration to header fileGetInternalformatParametermethod registration inConfigureFunctionTemplateGetInternalformatParametermethod in cpp fileImplementation Details:
Files Modified:
src/common/command_buffers/shared.hpp- Added command buffer type enumssrc/common/command_buffers/details/properties.hpp- Added request/response classes for command buffersrc/client/script_bindings/webgl/webgl2_rendering_context.hpp- Added method declarationsrc/client/script_bindings/webgl/webgl2_rendering_context.cpp- Added method registration and V8 binding implementationsrc/client/graphics/webgl_context.hpp- Added getInternalformatParameter method to WebGL2Context classsrc/client/graphics/webgl_context.cpp- Implemented command buffer query logicsrc/renderer/render_api_opengles.cpp- Implemented GL query handler with glGetInternalformativArchitecture:
Method Behavior (Full Spec Compliance):
glGetInternalformativ, returns values in descending order per specCode Quality:
Testing Notes:
WebGL2 Spec Compliance:
Review Feedback Addressed:
<issue_title>WebGL2: gl.getInternalformatParameter is not implemented (TypeError)</issue_title>
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.