Add indices

[pjcozzi]

Since glTF requires indices (CC #14), the converter needs to add indices for non-indexed:

• lines
• linestrips
• triangles
• trifans
• tristrips

See the Geometry section of the COLLADA spec

[fabrobinet]

These are actually types for mesh, and I must say I wasn't aware they can have 0 or 1 <p> i.e have indices or not.
My input in CC #14 was because I assumed that indices were required within COLLADA assets.
@RemiArnaud can you confirm this is not a bug in the spec ? are you aware of any exporter producing assets without <p> ?

[pjcozzi]

@fabrobinet are you saying that you believe that all primitive types in COLLADA require indices so that we might not have to do anything? (Except fix a COLLADA spec bug?)

[RemiArnaud]

do we really need the 'strips' anymore? that's old school no?

On Wed, Apr 17, 2013 at 11:00 AM, Patrick Cozzi notifications@git.luolix.topwrote:

@fabrobinet https://github.com/fabrobinet are you saying that you
believe that all primitive types in COLLADA require indices so that we
might not have to do anything? (Except fix a COLLADA spec bug?)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/60#issuecomment-16522120
.

[fabrobinet]

@RemiArnaud IMO, no not old school...why should we "unstrip" ? Stripped assets can be the result of long computation to squeeze indices size and make the walktrough indices more efficient, we should just pass through strips and preserve them. And..we are driven by WebGL/OpenGL specs, not our call to deprecate that.

[fabrobinet]

@pjcozzi Our plan was to have indices mandatory and wait for community feedback to see if we should relax this constraint for the ones willing to invoke drawArrays. But... I wasn't aware that COLLADA allowed non indexed primitives, and this surprises me a lot ! I was just wondering if that's a SPEC bug, anyhow I believe it is really a edge case. We should probably check other format like FBX, obj... before taking a decision

[pjcozzi]

@RemiArnaud IMO, no not old school...why should we "unstrip" ? Stripped assets can be the result of long computation to squeeze indices size and make the walktrough indices more efficient, we should just pass through strips and preserve them. And..we are driven by WebGL/OpenGL specs, not our call to deprecate that.

+1 since these don't add any real burden to the client. This is just an enum passed to drawElements.

However, I don't want to get too caught up in matching WebGL exactly; for example, I am really thinking about dropping dither from the render state since I've never seen a WebGL or OpenGL app that uses it. Perhaps I am too young? :)

[fabrobinet]

:)..

Agreed, I will not push for dither...

[RemiArnaud]

yes, both spec and schema says that

is optional.
the spec implies that if you have at least one in a primitive you need a

if you have no in a primitive, that means that all the are in the element

in other words, non indexed primitives would have an empty primitive element:

[fabrobinet]

@RemiArnaud Thanks for confirming that Remi.

Are you aware of any exporter no using indexed primitive ?
Our call with @pjcozzi was to just start with indexed primitive and wait community feedback to open up to non-indexed primitive ? What do you think ?

[RemiArnaud]

I have never seen a non indexed primitive out of an exporter so far.
But, I have seen a geometry without a primitive. This only defines vertices, which maps to points that can be used by the application for particles for example

[fabrobinet]

Yes, we thought about points too, but we can still work around with indices.
@RemiArnaud are you OK that we potentially support non indexed primitives post glTF 1.0 ?

[fabrobinet]

(and consider all community feedback in the meantime, between feature complete and 1.0..)

[RemiArnaud]

not sure why points should have indices, but also not sure glTF 1.0 need to
support points.

Note: linked to the other discussion about removing 'indices' and using an
attribute instead. If you have an 'INDEX' semantic, then the geometry is
indexed, if you don't then it is not indexed. Simple no?

On Tue, Apr 23, 2013 at 11:34 AM, Fabrice Robinet
notifications@git.luolix.topwrote:

(and consider all community feedback in the meantime, between feature
complete and 1.0..)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/60#issuecomment-16877400
.

[fabrobinet]

IMO not having an indices property for non indexed is equally simple :).

[RemiArnaud]

no as simple - instead of having to deal with all the glBindBuffers.. in the same loop (for all attributes), you have to do a special case in the code for the indices.

[fabrobinet]

it's a different target for VBO, so indices have to be a special case anyway.

[RemiArnaud]

can you explain the VBO case please?

[pjcozzi]

For this issue, it sounds like we will need to add support to the converter to add indices even though we expect just about all COLLADA models to have indices in practice. Anything else?

[fabrobinet]

no that's about it, let's continue to track this on the converter side.

[pjcozzi]

OK 👍

[pjcozzi]

@tfili how hard is this to add to the converter? Or perhaps we just not require indices in glTF, which would be trivial for the client, and better for point and line primitives.

[tfili]

I'd have to look at the code but I don't think it would be that hard. But if it's easy for the client to support and had a benefit, why not allow it?

[pjcozzi]

I am OK with not requiring indices in glTF. @tparisi?

[tparisi]

Requiring indices for non-indexed is wasteful; remember we're also trying to be efficient for transmission.

Do we have a clear proposal for non-indexed types?

[pjcozzi]

Do we have a clear proposal for non-indexed types?

mesh.primitive.indices (see here in the schema) would not be required in which case, the runtime would use drawArrays, not drawElements. I have to take a closer look to see where we would get the offset and count properties for drawArrays (probably as optional properties on mesh.primitive) since they would no longer come from the accessor. I'll follow-up with an example.

[mlimper]

Within our SRC Paper, we used count for all accessors, but we didn't consider offset at that point (see the example JSON at the end of the paper).

[pjcozzi]

@mlimper are you suggesting that we simply make mesh.primitive.indices optional, and when it is not provided, then the count from any accessor referenced from the primitive's attributes (which is the same for all of them) can be used in a call to drawArrays with offset = 0.

I think that would work just fine. Please confirm.

[mlimper]

I think that would work just fine. Please confirm.

Yes, confirmed! Thanks for the clarification.

[pjcozzi]

I updated the draft 1.0 schema in 740ac80.

@tfili I'm leaving this issue open and labeled as converter so you can update the converter (and Cesium loader).

@tparisi here's some text for the draft 1.0 spec (perhaps as an Implementation Note):

---

Primitive

Each primitive in mesh.primitives corresponds to one WebGL draw call (engines are, of course, free to batch draw calls). When primitive.indices is defined, it references the accessor to use for index data, and GL's drawElements function should be used. When primitive.indices is not defined, GL's drawArrays function should be used with a count equal to the count property of any of the accessors referenced by primitive.attributes (they are all equal for a given primitive).

[pjcozzi]

@tparisi I'm leaving this open and labeled converter for the converter changes.

[pjcozzi]

Discussed this with @tfili. It could be a few days due to an OpenCOLLADA issue so the converter work here may get pushed post 1.0 since these models are not common in the wild.

[RemiArnaud]

Is there an OpenCOLLADA Pull Request ?

[pjcozzi]

@tfili I am fine with dropping the post 1.0 label from this, updating the limitations wiki, and just doing it when it becomes priority.

If you agree, please remove the label and update the wiki.

[pjcozzi]

@tfili can you confirm that we can close this now since indices are not required?

[pjcozzi]

No longer an issue. indices are not required.

See https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/schema/mesh.primitive.schema.json#L16