[DOCS] Document APIs related to vertex attributes

[aidinabedi]

The description is updated to reflect the latest changes based on feedback.

The PR includes the following:

• Document property pc.VertexIterator#element.
• Document pc.VertexIteratorAccessor and its methods.

I confirm I have signed the Contributor License Agreement.

[aidinabedi]

@willeastcott Feel I need to add some background here since this PR includes code changes which I strongly want to avoid when my objective is to fix typings and documentation.

The biggest challenge I had with this PR was to document pc.VertexFormat#elements properly, since it shares a lot with parameter description (in the pc.VertexFormat constructor). Basically, I had two options:

1. Either document pc.VertexFormat#elements as-is and duplicate lots of existing documentation from the big object parameter description (in the pc.VertexFormat constructor).

2. Or rename a few properties in the pc.VertexFormat#elements object and share lots of jsdoc documentation thus avoiding duplication. This has also the side benefit of unifying the two objects pc.VertexFormat#elements and parameter description which improves API consistency and making it more intuitive.

I went with option 2 because I believe duplication will not only hurt readability and maintainability but also eventually lead to de-sync and confusing code documentation.

[willeastcott]

Just wanted to drop a quick comment in here. I've scanned the changes here and my reflex reaction is that I suspect this has the potential to break the Editor and certain PlayCanvas apps that leverage this part of the API. I'd have to do a more thorough investigation to weigh up the risks around that. But in short, I can't merge this as fast as your other PRs from yesterday.

[aidinabedi]

@willeastcott If you are wary about the code changes, I could go with option 1 as described in my previous comment:

1. Keep pc.VertexFormat#elements as-is and duplicate all the documentation from the object type parameter description (in the pc.VertexFormat constructor), but use pc.VertexFormat#elements specific property names.

[aidinabedi]

@willeastcott I've removed all code changes (by going with option 1) in the hopes getting this PR rolling :)

[aidinabedi]

@willeastcott Have you had the chance to take a new look at the PR?

[aidinabedi]

Hello @willeastcott,

I've reverted all jsdoc for the description param in the VertexFormat constructor and removed VertexAttributeDescription, as discussed.

Also simplified VertexAttributeElement by removing the constructor params (since it's never meant to be constructed by user code). And as mentioned in a previous comment, I still think there is a stronger need for VertexAttributeElement (because the object is shared among multiple engine classes).

Let me know if you have any more comments or thoughts.

[willeastcott]

Let me put my concern another way. After this PR is merged, I would consult the API ref and assume I can do this:

pc.app.root.findComponent('model').meshInstances[0].mesh.vertexBuffer.format.elements[0] instanceof pc.VertexAttributeElement

But that would return false.

[aidinabedi]

@willeastcott Fixed. I just thought we preferred avoiding code changes this time around :)

VertexAttributeElement is now a regular class and instantiated inside VertexFormat. The behavior and properties are all the same.

EDIT: PR description is also updated.

[aidinabedi]

@willeastcott Any chance we could merge and close this PR? :D

Let me know if there is anything else I can do to get the PR moving. I'm happy to address any remaining concerns. Adding proper documentation and typings for this important part of the API, would be very valuable to us and others.

[aidinabedi]

@willeastcott Would it help if I just removed VertexAttributeElement. I'm happy to do that if that is what it takes. That's probably what you've been trying to tell me this whole time :)

Or are there plans to deprecate this whole API? Is that why you're reluctant?

[willeastcott]

Yeah, sorry this PR is still in limbo. 😞

Actually, it's still pc.VertexAttributeElement that I'm not keen on. I think I've mentioned before, this PR is documented a new PlayCanvas class just to circumvent a deficiency in the JSDoc -> TSD toolchain. Sure, it's harder to fix the issue there, but that's where the problem lies, not PlayCanvas' API.

As for pc.VertexIteratorAccessor, that method of iterating vertices in vertex buffers is incredibly inefficient. It's pretty much legacy. We're in the process of developing a new API for fast creation and editing of procedural meshes: #1960 This part of the API will be even more irrelevant once we're done with that. I can imagine we might deprecate it entirely sometime soon. I feel polishing these APIs, adding more public classes etc, is just encouraging developers to use it, when actually we want to do the opposite.

[aidinabedi]

@willeastcott Thank you for responding.

I know you mentioned before that you're not keen on new classes just to fix typings. I responded by trying to argue that pc.VertexAttributeElement is different, but obviously failed :)

Anyhow, I hear you. I agree, iterating vertices in a interleaved vertex buffer is inefficient. The thing is, that interleaved data still considered to have better rendering performance. And from what I've seen in #1960, that PR will not provide a complete substitute for creating and accessing a interleaved buffer. And if a developer only needs to update/create a "procedural" mesh once, they should still have the option of choosing these "legacy" APIs for highest draw performance.

Additionally, as you know we're trying to write an glTF exporter for PlayCanvas. And having proper access to APIs like VertexIteratorAccessor is necessary to export vertex data efficiently without copying each attribute separately and re-creating the interleaved buffer ourselves in glTF.

[aidinabedi]

Alright, @willeastcott.

I've removed the new class VertexAttributeElement. This PR now only improves documentation for existing classes. No new classes just to circumvent a deficiency in tsd-jsdoc. PR description is updated.

I'm just making one existing class pc.VertexIteratorAccessor public, which is already used by devs and exposed by pc.VertexIterator.html#element, This PR only improves upon that existing documentation. Please consider it. Proper documentation and type-safety and avoids a lot of unnecessary bugs.

Additionally, as mentioned before, the new procedural mesh API won't completely replace the need for pc.VertexIteratorAccessor. Which the author of the new API also confirmed.

[willeastcott]

This looks in good shape now. But I have merge #1960 first - a word of warning: there will be some merge conflicts after that. But hopefully they'll be easy to resolve.

[willeastcott]

Thanks for your patience on this one, @aidinabedi. Feels great to finally merge it. 🎉