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

meshes vs instanceSkin #255

Closed
RemiArnaud opened this issue Mar 23, 2014 · 20 comments
Closed

meshes vs instanceSkin #255

RemiArnaud opened this issue Mar 23, 2014 · 20 comments

Comments

@RemiArnaud
Copy link
Contributor

This is related to #100, I thought it would be better to create an new issue to discuss a specific design matter rather than adding to #100.

In node, we can have meshes and instanceSkin

Design wise, I think we can improve this bringing better uniformity. Basically, I think we either should have instanceMesh and instanceSkin, or we can have meshes and skins in node.

Basically an instanceXX - as in instanceProgram - enable referencing a shared/read-only object and augmenting or specializing it with additional parameters. so far we have made node reference specific objects, and not use instances. This may create a bit of duplication, but its a simpler design, and so far the duplications have been minor, and it IMHO it fits better a run-time format.

So, could we change a bit the skin design, so that node can reference a specific skin, rather than using a instanceSkin?

Other questions: can we have both skins and meshes in a node?
Also noticed: a node can have many meshes (meshes: []) but only one instanceSkin ?

@RemiArnaud
Copy link
Contributor Author

The proposal would be to move skeletons and sources in skin.
(which means that some skins may have to be duplicated in some cases, but the size is quite small)
One advantage is that it make it easier to parse gltf json, as all the nodes can be parsed first, and then the skins - so the skins references to skeletons and jointID can be resolved before parsing skins.
Otherwise, as it is now, the instanceSkin does forward references into the nodes.

@pjcozzi
Copy link
Member

pjcozzi commented Mar 24, 2014

@RemiArnaud can you provide a concrete before/after JSON example? @fabrobinet will be the best person to provide feedback, but this is the most confusing part of glTF to me (perhaps necessarily so) so I am interested in anything that will simplify it.

Other questions: can we have both skins and meshes in a node?

From the schema:

"A node will have either the camera, light, meshes, or instanceSkin property defined"

@pjcozzi pjcozzi added this to the Draft 1.0 spec milestone Apr 30, 2014
@pjcozzi
Copy link
Member

pjcozzi commented Nov 25, 2014

@fabrobinet for 1.0?

@fabrobinet
Copy link
Contributor

I am not comfortable agreeing or punting this without a concrete schema proposal, @RemiArnaud can you write it down so that there is no ambiguity ?

@pjcozzi
Copy link
Member

pjcozzi commented Aug 27, 2015

If we want to consider this for 1.0, please provide an concrete JSON schema proposal.

@pjcozzi pjcozzi removed this from the Spec 1.0 milestone Aug 27, 2015
@pjcozzi
Copy link
Member

pjcozzi commented Sep 17, 2015

Seeing no proposals, closing.

@pjcozzi pjcozzi closed this as completed Sep 17, 2015
@tparisi tparisi reopened this Sep 17, 2015
@tparisi
Copy link
Contributor

tparisi commented Sep 17, 2015

Hold on, wasn't this really just a naming thing? Why not just go with meshes and skin as the property names on the node (vs instanceSkin)

In general I think we could ditch the 'instance' naming and simplify things. A material could have a technique property instead of instanceTechnique-- you and I just discussed this earlier today.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 17, 2015

I'm not sure it is as simple as a rename, I think there is a semantic difference. @fabrobinet any chance you recall?

If this is just a rename, then, yes, I am all for dropping the instance prefix.

@tparisi
Copy link
Contributor

tparisi commented Sep 17, 2015

Beyond the rename, I don't think we want to contemplate a redesign right now.

Do you like the technique rename idea too?

@pjcozzi
Copy link
Member

pjcozzi commented Sep 17, 2015

Do you like the technique rename idea too?

Yes, but let's see the schema proposal.

@tparisi
Copy link
Contributor

tparisi commented Sep 17, 2015

For which, the new materials/techniques/common design?

@pjcozzi
Copy link
Member

pjcozzi commented Sep 17, 2015

I'm not sure what you are talking about anymore, but let's keep all material discussion in #84.

@pjcozzi
Copy link
Member

pjcozzi commented Oct 9, 2015

For 1.0 for this issue I suggest we either

(1) do nothing, or
(2) "flatten" instanceSkin into node (like we did with the other instance properties for program and technique), e.g.,

Before

        "Geometry-mesh019Node": {
            "children": [],
            "instanceSkin": {
                "meshes": [
                    "Geometry-mesh019"
                ],
                "skeletons": [
                    "locator047Node",
                    "locator051Node"
                ],
                "skin": "Controller-mesh019"
            },
            "matrix": [...],
            "name": "Cesium_Man"
        }

After

        "Geometry-mesh019Node": {
            "children": [],
            "meshes": [
                "Geometry-mesh019"
             ],
            "skeletons": [
                "locator047Node",
                "locator051Node"
            ],
            "skin": "Controller-mesh019",
            "matrix": [...],
            "name": "Cesium_Man"
        }

This explicitly implies that a node can't have both a skin and meshes (for rendering), which we already say in the schema (#255 (comment)).

@tparisi @tfili what do you think? I'm OK with either option. Let's try to converge ASAP.

@tparisi
Copy link
Contributor

tparisi commented Oct 9, 2015

In general I am for simplifying

Tony Parisi
[email protected]
415.902.8002

On Oct 9, 2015, at 10:14 AM, Patrick Cozzi [email protected] wrote:

For 1.0 for this issue I suggest we either

(1) do nothing, or
(2) "flatten" instanceSkin into node (like we did with the other instance properties for program and technique), e.g.,

Before

    "Geometry-mesh019Node": {
        "children": [],
        "instanceSkin": {
            "meshes": [
                "Geometry-mesh019"
            ],
            "skeletons": [
                "locator047Node",
                "locator051Node"
            ],
            "skin": "Controller-mesh019"
        },
        "matrix": [...],
        "name": "Cesium_Man"
    }

After

    "Geometry-mesh019Node": {
        "children": [],
        "meshes": [
            "Geometry-mesh019"
         ],
        "skeletons": [
            "locator047Node",
            "locator051Node"
        ],
        "skin": "Controller-mesh019",
        "matrix": [...],
        "name": "Cesium_Man"
    }

This explicitly implies that a node can't have both a skin and meshes (for rendering), which we already say in the schema (#255 (comment)).

@tparisi @tfili what do you think? I'm OK with either option. Let's try to converge ASAP.


Reply to this email directly or view it on GitHub.

@tfili
Copy link

tfili commented Oct 9, 2015

I'm good with simplifying as well.

@pjcozzi
Copy link
Member

pjcozzi commented Oct 9, 2015

OK, let's do it.

@tfili
Copy link

tfili commented Oct 9, 2015

Updated converter in dev-1.0 at fb40b4c

@pjcozzi
Copy link
Member

pjcozzi commented Oct 10, 2015

@tparisi please close this when the spec is updated.

tparisi added a commit that referenced this issue Oct 12, 2015
@tparisi tparisi closed this as completed Oct 12, 2015
@tparisi
Copy link
Contributor

tparisi commented Oct 12, 2015

@pjcozzi please review the language thanks

@pjcozzi
Copy link
Member

pjcozzi commented Oct 12, 2015

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants