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

Refactor docs to separate WebGL v1 and v2 content. #32613

Merged
merged 7 commits into from
Mar 13, 2024
Merged

Refactor docs to separate WebGL v1 and v2 content. #32613

merged 7 commits into from
Mar 13, 2024

Conversation

tsnee
Copy link
Contributor

@tsnee tsnee commented Mar 8, 2024

Description

WebGLRenderingContext and WebGL2RenderingContext have overloaded versions of the bufferData and bufferSubData functions. Before, all were documented under files/en-us/web/api/webglrenderingcontext. After a discussion on a previous PR it was decided to split them up.

Motivation

As things stand today, webgl2renderingcontext/index.md contains broken links. This PR creates the files required to resolve those links.

Related issues and pull requests

Relates to #32598

@tsnee tsnee requested a review from a team as a code owner March 8, 2024 13:20
@tsnee tsnee requested review from wbamberg and removed request for a team March 8, 2024 13:20
@github-actions github-actions bot added Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Mar 8, 2024
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thank you @tsnee ! Like I say, I'm no expert on WebGL so I'd appreciate your checking my comments, which are just based on a read of the spec syntax.

Comment on lines 18 to 20
bufferData(target, usage, srcOffset)
bufferData(target, srcData, usage, srcOffset)
bufferData(target, srcData, usage, srcOffset, length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I read the specs we've got the following definitions:

undefined bufferData(GLenum target, GLsizeiptr size, GLenum usage);
undefined bufferData(GLenum target, AllowSharedBufferSource? srcData, GLenum usage);
undefined bufferData(GLenum target, [AllowShared] ArrayBufferView srcData, GLenum usage,
    unsigned long long srcOffset, optional GLuint length = 0);

So it looks to me as if:

  • either size or srcData must be given but not both
  • srcOffset is effectively optional but is only allowed if srcData is given
  • length is optional but is only allowed if srcOffset is given

...which would give us the following syntax variations:

bufferData(target, size, usage)
bufferData(target, srcData, usage)
bufferData(target, srcData, usage, srcOffset)
bufferData(target, srcData, usage, srcOffset, length)

Do you think that is right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Comment on lines -21 to -25

// WebGL2
bufferData(target, usage, srcOffset)
bufferData(target, srcData, usage, srcOffset)
bufferData(target, srcData, usage, srcOffset, length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't leave a suggestion but "// WebGL1" above should be removed as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -18,11 +18,6 @@ buffer object's data store.
bufferData(target, usage)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks to me like either size or srcData must be provided, so this version should be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

// WebGL2
bufferData(target, usage, srcOffset)
bufferData(target, srcData, usage, srcOffset)
bufferData(target, srcData, usage, srcOffset, length)
```

### Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't comment below but maybe mark size and srcData optional and say one of them must be provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accepted your suggestion.

@@ -18,11 +18,6 @@ object's data store.
// WebGL1
bufferSubData(target, offset)
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this option is not valid, from the spec?

undefined bufferSubData(GLenum target, GLintptr offset, AllowSharedBufferSource data);

(also it's called data not srcData in the spec, not sure this is worth fixing.)

@tsnee
Copy link
Contributor Author

tsnee commented Mar 9, 2024

Like I say, I'm no expert on WebGL so I'd appreciate your checking my comments, which are just based on a read of the spec syntax.

I am not either. Your comments look correct to me, but I don't know how to prove they are right. I am only just beginning to work with WebGL. I will see if I can write some sample code to find out what works and what does not. However I would feel a lot more confident if a subject matter expert could weigh in.

@davesmith00000
Copy link

Hi! @tsnee and I have been discussing some related work in the background for a ticket I raised (to do with WebGL2) on another repo, that has resulted in this valiant effort.
I wouldn't call myself a subject matter expert, and would happily bow to a higher authority if one showed up, but I have spent a good amount of time with the Kronos docs and translating them to usable interfaces.
All that said, @wbamberg's comments all look correct and reasonable to me, based on the Kronos docs links provided.
Hope that helps, great work all.

tsnee and others added 2 commits March 10, 2024 05:55
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fix linting problem.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for the update @tsnee and for the review @davesmith00000 !

The only thing left here is to delete lines 17 and 18 from https://github.com/mdn/content/pull/32613/files#diff-f3fa71864e6aba88eba045c4a46156dcd01acefa12cdef0e0d1a547168ebc1c8

// WebGL1
bufferData(target, usage)

.per the comments at https://github.com/mdn/content/pull/32613/files#r1518443366 and https://github.com/mdn/content/pull/32613/files#r1518441714.

After that we should be good I think.

@tsnee
Copy link
Contributor Author

tsnee commented Mar 13, 2024

Oops, I thought I had already done that. I'm glad you double checked.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

🎉 thank you for your work on this!

@wbamberg wbamberg merged commit 35f5a02 into mdn:main Mar 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants