Implement WebGL2 getInternalformatParameter with full spec compliance#416
Implement WebGL2 getInternalformatParameter with full spec compliance#416
Conversation
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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; | ||
| } |
There was a problem hiding this comment.
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.
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. |
|
@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
| } | ||
| if (values.empty()) | ||
| { | ||
| DEBUG(DEBUG_TAG, "[GetInternalformatParameter] Using fallback values {4, 2, 1}"); |
There was a problem hiding this comment.
@copilot Other paramter names could be implemented, too.
There was a problem hiding this comment.
Added support for GL_NUM_SAMPLE_COUNTS in commit 51f300a. The implementation now supports both WEBGL_SAMPLES and GL_NUM_SAMPLE_COUNTS parameter names. Additional pnames can be easily added following the same pattern if needed.
- Added GL_NUM_SAMPLE_COUNTS constant (0x9380) to webgl_constants.hpp - Implemented GL_NUM_SAMPLE_COUNTS query in OnGetInternalformatParameter - Now supports both WEBGL_SAMPLES and GL_NUM_SAMPLE_COUNTS pnames - Updated comment to reflect support for multiple parameter names - Returns count of available sample levels for GL_NUM_SAMPLE_COUNTS Addresses feedback from @yorkie to implement other parameter names. Co-authored-by: yorkie <1935767+yorkie@users.noreply.github.com>
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/common/command_buffers/webgl_constants.hpp- Added GL_NUM_SAMPLE_COUNTS constantsrc/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.