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

Remove the max 1.0 restriction on emissiveFactor #1193

Closed
wants to merge 1 commit into from

Conversation

emackey
Copy link
Member

@emackey emackey commented Dec 29, 2017

Fixes #1083

I came across a model using 1.5 for the emissiveFactor, seems to work fine in the JS engines I normally test (Cesium, Babylon, ThreeJS).

Note however, Windows 10's apps (I tested Paint 3D, Mixed Reality Viewer) are very strict about this value, and will reject models that contain >1.0 emissiveFactor. Is it OK for us to remove a restriction from the spec like this?

(The model I tested with was "blackvan" from AnalyticalGraphicsInc/gltf-vscode#80).

/cc @bghgary @sbtron

@pjcozzi
Copy link
Member

pjcozzi commented Dec 30, 2017

Does the .md file also need to be updated?

@emackey
Copy link
Member Author

emackey commented Dec 31, 2017

@emackey
Copy link
Member Author

emackey commented Dec 31, 2017

Although it looks like alphaCutoff has one, maybe this is already out of sync?

EDIT: Looks like that's due to wetzel handling min/max numbers, but not min/max for elements of an array.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 31, 2017

@emackey do you want to regenerate that part of the .md or update by hand?

@emackey
Copy link
Member Author

emackey commented Dec 31, 2017

It would need a wetzel feature addition (min/max for array values). I did some wetzel cleanup this morning but I don't think I'll have a chance to get to this one.

@javagl
Copy link
Contributor

javagl commented Jan 1, 2018

"Off-topic": The task to familiarize myself with wetzel (in order to fix #1013 ) has been pending for far too long now (only justified by "not having highest priority"), but maybe I can have a look at this issue as well when I get the chance. I'll add it to the TODO list...

@bghgary
Copy link
Contributor

bghgary commented Jan 2, 2018

@emackey

Is it OK for us to remove a restriction from the spec like this?

I don't think this is okay. We can't change the schema like this and still meet versioning requirements. For example, older versions of native apps that use the old schema to validate will fail to load these new assets.

The ways I can see this working is if we add another factor value that either replaces the old value if recognized or multiplies against it. The old emissiveFactor must continue to be populated with a reasonable fallback value for older clients.

@zellski
Copy link
Contributor

zellski commented Jan 2, 2018

I agree changing the schema is dangerous. As @bghgary says, it's important that assets that were previously invalid aren't retroactively declared spec-compliant. A validating loader that could claim to handle all compliant content yesterday should not find themselves partial implementations today — not without a new schema revision.

@emackey emackey closed this Jan 2, 2018
@emackey
Copy link
Member Author

emackey commented Jan 2, 2018

Closed this. We should maybe clarify #1083 (comment) to make it clear this is not as simple as removing the 1.0 restriction.

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

Successfully merging this pull request may close these issues.

None yet

5 participants