Skip to content

/ servo

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

Add initial support for VertexAttribI4* and VertexAttribIPointer #26123

Closed
wants to merge 1 commit into from
Closed

Add initial support for VertexAttribI4* and VertexAttribIPointer #26123

wants to merge 1 commit into from

Conversation

@imiklos
Copy link
Contributor

imiklos commented Apr 6, 2020

Add initial support for the WebGL2 VertexAttribI4i, VertexAttribI4iv, VertexAttribI4ui, VertexAttribI4uiv and VertexAttribIPointer calls.

I've already updated the test expectations and noticed that I've got a crash on the conformance2/state/gl-object-get-calls.html test, which is related to the GetFragDataLocation.
The crash comes from here:

servo/components/canvas/webgl_thread.rs

Line 1292 in b4d7ec1

assert!(location >= 0);

The assert is invalid here because the GetFragDataLocation GL call could return with -1 also. (https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glGetFragDataLocation.xhtml)

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

cc @mmatyas @zakorgy @jdm
@highfive
Copy link

highfive commented Apr 6, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webglvertexarrayobject.rs, components/script/dom/webglvertexarrayobjectoes.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/vertexarrayobject.rs, components/script/dom/webglrenderingcontext.rs and 1 more
  • @KiChjang: components/script/dom/webglvertexarrayobject.rs, components/script/dom/webglvertexarrayobjectoes.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/vertexarrayobject.rs, components/script/dom/webglrenderingcontext.rs and 1 more
@highfive
Copy link

highfive commented Apr 6, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm
jdm requested changes Apr 6, 2020
View changes
Copy link
Member

jdm left a comment

Thanks for implementing this!
components/script/dom/webgl2renderingcontext.rs Outdated

let current_vertex_attrib =
self.base.current_vertex_attribs()[prog_attrib.location as usize];
let attrib_data_base_type = if attrib.enabled_as_array {

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Apr 6, 2020

Member

This check looks backward to me, and when I invert it then all the test failures in attrib-type-match.html and gl-vertex-attrib-i-render.html disappear for me.

This comment has been minimized.

Sign in to view
Copy link
@imiklos

imiklos Apr 7, 2020

Author Contributor

Thank you! You are right! I've updated the test expectations but the test failures did not disappear on my side. Could it be driver related?
@jdm
Copy link
Member

jdm commented Apr 6, 2020 •
edited

I agree with you that the assert for glGetFragDataLocation should be removed.
@jdm jdm assigned jdm and unassigned SimonSapin Apr 6, 2020
@imiklos
Add initial support for VertexAttribI4*, VertexAttribIPointer
Loading status checks…
08279ee 
Adds initial support for the WebGL2 `VertexAttribI4i`, `VertexAttribI4iv`, `VertexAttribI4ui`, `VertexAttribI4uiv` and `VertexAttribIPointer` calls.
@imiklos imiklos force-pushed the szeged:vertex_attrib_i4 branch from 234cc11 to 08279ee Apr 7, 2020
@imiklos
Copy link
Contributor Author

imiklos commented Apr 7, 2020

@jdm Thank you for the review! I've removed the assert from the glGetFragDataLocation, now on the gl-object-get-calls.html test I get an Unexpected WebGL error from the GL side ( related to the GetActiveUniformBlockName ) and the test is timing out even when I use --timeout-multiplier.
@jdm
Copy link
Member

jdm commented Apr 7, 2020

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Apr 7, 2020

⌛ Trying commit 08279ee with merge 5f83300...
bors-servo added a commit that referenced this pull request Apr 7, 2020
@bors-servo
Auto merge of #26123 - szeged:vertex_attrib_i4, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5f83300 
Add initial support for VertexAttribI4* and VertexAttribIPointer

Add initial support for the WebGL2 `VertexAttribI4i`, `VertexAttribI4iv`, `VertexAttribI4ui`, `VertexAttribI4uiv` and `VertexAttribIPointer` calls.

<!-- Please describe your changes on the following line: -->
I've already updated the test expectations and noticed that I've got a crash on the `conformance2/state/gl-object-get-calls.html` test, which is related to the `GetFragDataLocation`.
The crash comes from here: https://github.com/servo/servo/blob/b4d7ec1c99259f936c9e34de157b652707d308de/components/canvas/webgl_thread.rs#L1292
The assert is invalid here because the `GetFragDataLocation` GL call could return with `-1` also. (https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glGetFragDataLocation.xhtml)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] There are tests for these changes

cc @mmatyas @zakorgy @jdm
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 7, 2020

☀️ Test successful - status-taskcluster
State: approved= try=True
@jdm
Copy link
Member

jdm commented Apr 7, 2020

The GetActiveUniformBlockName panic is caused by #26134.
@jdm jdm mentioned this pull request Apr 7, 2020
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this pull request Apr 7, 2020
@bors-servo
Auto merge of #26135 - jdm:vertex_attrib_i4, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
10a9cab 
Add initial support for VertexAttribI4* and VertexAttribIPointer

Add initial support for the WebGL2 `VertexAttribI4i`, `VertexAttribI4iv`, `VertexAttribI4ui`, `VertexAttribI4uiv` and `VertexAttribIPointer` calls.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26134 and fix #26123.
- [x] There are tests for these changes
bors-servo added a commit that referenced this pull request Apr 7, 2020
@bors-servo
Auto merge of #26135 - jdm:vertex_attrib_i4, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
4f0e43d 
Add initial support for VertexAttribI4* and VertexAttribIPointer

Add initial support for the WebGL2 `VertexAttribI4i`, `VertexAttribI4iv`, `VertexAttribI4ui`, `VertexAttribI4uiv` and `VertexAttribIPointer` calls.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26134 and fix #26123.
- [x] There are tests for these changes
@bors-servo bors-servo closed this in 7b5ec99 Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
S-awaiting-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@imiklos @highfive @jdm @bors-servo @SimonSapin
You can’t perform that action at this time.