Skip to content
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

Document pipelines #724

Merged
merged 6 commits into from
May 4, 2020
Merged

Document pipelines #724

merged 6 commits into from
May 4, 2020

Conversation

kvark
Copy link
Contributor

@kvark kvark commented Apr 21, 2020

Relevant investigations: #26, #267
There is more work to be done here, like describing all of the render states, maybe having a separate rasterization section. I don't think we should hold it until everything is done - landing in small chunks is easier.
Also, this is a rough draft, I need to double-check how this is specified in the native APIs and likely extend that.


Preview | Diff

@kvark kvark changed the title Document pipeline layouts Document pipelines Apr 22, 2020
@kvark kvark requested review from JusSn and kainino0x April 22, 2020 18:55
@kvark kvark added this to Needs Specification in Main Apr 22, 2020
@kvark kvark marked this pull request as ready for review April 22, 2020 18:58
Copy link
Contributor

@Kangz Kangz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, this doesn't need to be perfecto to be merged and we can iterate in follow up PRs.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kvark
Copy link
Contributor Author

kvark commented Apr 27, 2020

@Kangz thank you for the review!
I believe your concerns are addressed. I described the detailed equivalence rules. The extra limits, as well as proper reference to them, should be left for other PRs.

Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partial review

spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved

Only the lower {{GPURenderPipelineDescriptor/sampleCount}} bits of the mask are considered.

If the least-significant bit at position |N| of the [=final sample mask=] has value of "0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final sample mask is a single number, right? What does the LSB at position N of that mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mask is a number, the LSB at position N is a bit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that just "the bit at position |N|"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much, yes. I just wanted to be more specific and tell where the position is counting from, i.e. "least-significant" part

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. This sounded like "Take the thing at position N, then take the LSB of that" but maybe word this as "If the Nth-least-significant bit"?

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
@kvark
Copy link
Contributor Author

kvark commented Apr 28, 2020

Thanks @kainino0x , everything is addressed now!

@kainino0x
Copy link
Contributor

I reopened one of the threads, but feel free to merge anytime

@kvark
Copy link
Contributor Author

kvark commented Apr 30, 2020

@JusSn would you want to have a look?

@kvark
Copy link
Contributor Author

kvark commented May 4, 2020

@JusSn please review this post-landing!

@kvark kvark merged commit bc902fb into gpuweb:master May 4, 2020
@kvark kvark deleted the pipeline branch May 4, 2020 21:03
JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Jun 8, 2020
* Document pipeline layouts

* Document pipelines

* Vertex buffer and attribute validation

* Detail the rules of bind group equivalence

* Refactor algorithm definitions

* Indentation fixes
JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Jun 8, 2020
* Document pipeline layouts

* Document pipelines

* Vertex buffer and attribute validation

* Detail the rules of bind group equivalence

* Refactor algorithm definitions

* Indentation fixes
@kainino0x kainino0x moved this from Needs Specification to Specification Done in Main Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Main
Specification Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants