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

Implement GPURenderPassEncoder #26769

Merged
merged 2 commits into from Jun 4, 2020
Merged

Implement GPURenderPassEncoder #26769

merged 2 commits into from Jun 4, 2020
+572 −115

Conversation

@kunalmohan
Copy link
Collaborator

kunalmohan commented Jun 3, 2020

Add webidls for GPURenderPassEncoder and GPURenderEncoderBase and
implement relevant methods.

r?@kvark

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented Jun 3, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/GPURenderEncoderBase.webidl, components/script/dom/gpurenderpipeline.rs, components/script/dom/webidls/GPURenderPassEncoder.webidl, components/script/dom/gpucomputepassencoder.rs, components/script/dom/gpudevice.rs and 5 more
  • @KiChjang: components/script/dom/webidls/GPURenderEncoderBase.webidl, components/script/dom/gpurenderpipeline.rs, components/script/dom/webidls/GPURenderPassEncoder.webidl, components/script/dom/gpucomputepassencoder.rs, components/script/dom/gpudevice.rs and 5 more
@highfive
Copy link

highfive commented Jun 3, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 3, 2020 

error[E0432]: unresolved import `crate::dom::bindings::codegen::UnionTypes::GPULoadOpOrRangeEnforcedUnsignedLong`
  --> components/script/dom/gpucommandencoder.rs:13:5
   |
13 |     GPULoadOpOrRangeEnforcedUnsignedLong as GPULongLoad,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `GPULoadOpOrRangeEnforcedUnsignedLong` in `dom::bindings::codegen::UnionTypes`

This did not show up during my local build :(

The thing is I have this dictionary in GPUCommandEncoder.webidl

dictionary GPURenderPassDepthStencilAttachmentDescriptor {
    required GPUTextureView attachment;

    required (GPULoadOp or float) depthLoadValue;
    required GPUStoreOp depthStoreOp;
    boolean depthReadOnly = false;

    required (GPULoadOp or GPUStencilValue) stencilLoadValue;
    required GPUStoreOp stencilStoreOp;
    boolean stencilReadOnly = false;
};

I need to match on stencilLoadValue and depthLoadValue for which I need to import their types. Is there someplace else I should import them?
@jdm
Copy link
Member

jdm commented Jun 3, 2020

This is really strange.
@bors-servo try=linux
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

⌛ Trying commit 4b6316c with merge 387495a...
bors-servo added a commit that referenced this pull request Jun 3, 2020
@bors-servo
Auto merge of #26769 - kunalmohan:gpu-render-pass, 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…
387495a 
Implement GPURenderPassEncoder

Add webidls for GPURenderPassEncoder and  GPURenderEncoderBase and
implement relevant methods.

<!-- Please describe your changes on the following line: -->
r?@kvark

---
<!-- 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
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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 Jun 3, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Jun 3, 2020

It makes no sense to me that your local build does not reproduce this build error. My only advice at this point is to do the following:

  • make a build error occur in one of gpuadapter.rs by removing an argument from one of the trait methods
  • using the path to the generated file that is present in the error message you receive, find the generated UnionTypes.rs that is in the generated bindings' parent directory
  • gist that file so we can better understand if GPULoadOpOrRangeEnforcedUnsignedLong is actually present or not
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 3, 2020

@jdm https://gist.github.com/kunalmohan/59b4c4dc2beb3fb789da2ed2507a9bcf
@kvark
kvark approved these changes Jun 3, 2020
View changes
Copy link
Member

kvark left a comment

Looks great! I have a few questions, no blocking concerns.
components/script/dom/gpucomputepassencoder.rs Show resolved Hide resolved
components/script/dom/gpucomputepassencoder.rs Outdated Show resolved Hide resolved
components/script/dom/gpurenderpassencoder.rs Outdated Show resolved Hide resolved
components/script/dom/gpurenderpassencoder.rs Outdated Show resolved Hide resolved
@jdm
Copy link
Member

jdm commented Jun 3, 2020 •
edited

Super weird. I built your branch locally and don't see the build error. What OS are you using? I'm on macOS, and it builds successfully with both python 2 and python 3.
@jdm
Copy link
Member

jdm commented Jun 3, 2020

@bors-servo try=mac retry
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

⌛ Trying commit 4b6316c with merge 4a17df6...
bors-servo added a commit that referenced this pull request Jun 3, 2020
@bors-servo
Auto merge of #26769 - kunalmohan:gpu-render-pass, 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…
4a17df6 
Implement GPURenderPassEncoder

Add webidls for GPURenderPassEncoder and  GPURenderEncoderBase and
implement relevant methods.

<!-- Please describe your changes on the following line: -->
r?@kvark

---
<!-- 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
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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. -->
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 3, 2020 •
edited

I am on Ubuntu 18.04. I haven't tried building with python3.
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

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

jdm commented Jun 3, 2020

As a possible workaround, does anything change if you make a typedef for that union and import the typedef name in rust instead?
@kunalmohan
Implement GPURenderPassEncoder
Loading status checks…
1d4efb4 
Add webidls for GPURenderPassEncoder and  GPURenderEncoderBase and
implement relevant methods.
@kunalmohan kunalmohan force-pushed the kunalmohan:gpu-render-pass branch from 4b6316c to 1d4efb4 Jun 3, 2020
@highfive highfive removed the S-tests-failed label Jun 3, 2020
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 3, 2020

I have modified that particular type to use typedef, as suggested, to check if the build succeeds.
I haven't yet addressed all the comments from the review yet- wgpu-core is yet to be updated- so please don't merge.
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 3, 2020

So build succeeds now \o/. I don't know why that particular type was throwing the error while the other two didn't (Maybe it was cursed :P).
I'll proceed with this if that's not a problem, and push the other changes too.
@kunalmohan
Update wgpu-core and wgpu-types
Loading status checks…
3b5ede1
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 3, 2020

@kvark PTAL!
@kunalmohan kunalmohan added this to In progress in WebGPU MVP via automation Jun 3, 2020
@kvark
kvark approved these changes Jun 3, 2020
View changes
@jdm
Copy link
Member

jdm commented Jun 3, 2020

@bors-servo r=kvark
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

📌 Commit 3b5ede1 has been approved by kvark
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

⌛ Testing commit 3b5ede1 with merge 643c614...
bors-servo added a commit that referenced this pull request Jun 3, 2020
@bors-servo
Auto merge of #26769 - kunalmohan:gpu-render-pass, r=kvark
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…
643c614 
Implement GPURenderPassEncoder

Add webidls for GPURenderPassEncoder and  GPURenderEncoderBase and
implement relevant methods.

<!-- Please describe your changes on the following line: -->
r?@kvark

---
<!-- 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
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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 Jun 4, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Jun 4, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

⌛ Testing commit 3b5ede1 with merge 0281ace...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

☀️ Test successful - status-taskcluster
Approved by: kvark
Pushing 0281ace to master...
@bors-servo bors-servo merged commit 0281ace into servo:master Jun 4, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
WebGPU MVP automation moved this from In progress to Done Jun 4, 2020
@kunalmohan kunalmohan deleted the kunalmohan:gpu-render-pass branch Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvark kvark

Assignees

@kvark kvark

Labels
None yet
Projects
WebGPU MVP
  
Done
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

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