Comments (10)
Going to close this since I don't have bandwidth to work on this, I'll leave it up to the maintainers of this project to prioritize extension support.
from gltf-sdk.
Sorry again for the slow response @zeux. This sounds reasonable to me. Chatted with @agrittmsft a bit. The main concern with changes like this is that we can't break backwards compatibility. Perhaps add a flag similar to this so that SDK users have to opt-in to this new behavior? Feel free to submit a PR so we can discuss the changes further.
from gltf-sdk.
There's two questions I'd like to clarify on the backwards compatibility, and one on the way to configure this, before implementing it.
-
Right now, there are some glTF files that are invalid as per spec but are accepted. For example, the colors are always read assuming the attributes are normalized. Should fixing that have a flag? Should it be part of this change at all?
-
Right now, glTF files that require KHR_mesh_quantization and use e.g. integer position data, are valid as per spec (with a draft extension), but are not accepted. Should fixing that have a flag?
-
Assuming answer to #2 is "yes", should the flag be specified as part of the deserialization flags, or is there a way to specify them as part of the extension loader somehow? Is there an existing precedence within this library of opting into a specific extension?
The reason why I'm asking these questions is that backwards compatibility, as classically understood, refers to being able to load previously valid files, may refer to being able to load previously loadable files that aren't valid, but it's odd to see this in context of enabling the use of files that previously could not be loaded.
That, and I really don't want to write any more code for this before understanding what code is likely to be accepted...
from gltf-sdk.
I really don't want to write any more code for this before understanding what code is likely to be accepted
I understand.
For 1 and 2, I think you can gate all of the changes in one flag. Users of the SDK will read the extension and pass this extra flag to update to a new behavior for validation. At some point, we may be able to deprecate the old way, but it will take time for products to catch up.
For 3, I don't think there is precedence on this. In any case, the deserialization flags are not for validation, so it probably makes the most sense to add a new set of flags for the validation step.
Fixing anything that changes the behavior of whether something is accepted or rejected needs to be carefully considered. For example, say a product (e.g. PowerPoint) uses this SDK to persist the resulting glTF in its file container. If those files can no longer load after updating to the latest glTF-SDK, then that will be an issue. We have to be super careful.
from gltf-sdk.
probably makes the most sense to add a new set of flags for the validation step.
The issue with that though is that right now validation for geometry attributes is actually not performed by the Validate() function - it's performed by the utility functions that read the geometry data. If that code stays there, it will continue to reject the inputs.
If those files can no longer load after updating to the latest glTF-SDK, then that will be an issue. We have to be super careful.
Yes, but that's a different dimension from 2, right? It's similar to 1 - if right now glTF-SDK accepts invalid glTF files, fixing that may break backwards compat. But 2 doesn't involve rejecting any files that are currently accepted, it's the opposite.
from gltf-sdk.
Maybe a concrete example would help illustrate my confusion around my point 2. When KHR_texture_transform support was added in #31, glTF-SDK started parsing files where that extension was required. However, this didn't require the users of the SDK opting into the support of this extension. And I don't understand why KHR_mesh_quantization is treated differently.
from gltf-sdk.
not performed by the Validate() function
Oh right, sorry about that. I need to page this all back in. We are talking about the static utility functions. But it's the same issue. Changing the behavior of these function breaks back compat.
2 doesn't involve rejecting any files that are currently accepted, it's the opposite.
That can still cause issues for existing code. If more things are accepted than before, existing code that isn't expecting those things to come through may have problems.
why KHR_mesh_quantization is treated differently
I haven't looked at #31 super carefully, but I don't think the existing behavior changes. A change to the utility functions to accept more values has a possibility of breaking existing code.
Perhaps we can add a static flag for this, add a flag per function, or create a separate set of static functions? If we decide on the latter, I think the code can be factored so that the new set of functions reuse most of the code.
from gltf-sdk.
Ok, so a possible proposal - please correct me if I'm wrong - could be:
- Expand Validation function to validate the geometry attributes in accordance to the spec & the presence of KHR_mesh_quantization extension
- Change existing utility functions to use Validation function(s) instead of existing logic, so that the validation is centralized
- Make sure that all of these changes are gated behind some flag, possibly a static runtime flag, so that applications can independently upgrade to the new version of the SDK and enable that flag to actually observe the behavior differences
Does that sound reasonable?
And I really think that this project should have a clear guideline for adding new extensions, because this process isn't what I'd normally expect a glTF reader to follow. It's up to you as to what that guideline is, but in absence of the documented guideline the natural choice is different from what's being discussed.
That is,
I haven't looked at #31 super carefully, but I don't think the existing behavior changes. A change to the utility functions to accept more values has a possibility of breaking existing code.
Unless I am missing something, for example files with KHR_texture_transform extension that have offset with just 1 element would load just fine before this change, and would throw on load with that change. So an invalid glTF file that was previously okay to load would fail to load.
I was also initially under the impression that files with KHR_texture_transform in the extensionsRequired list would fail to load before that PR and not after that PR, but it looks like this SDK doesn't act on extensionsRequired contents so that's not a problem (although that is in stark contrast with other validation decisions made by this SDK).
from gltf-sdk.
Does that sound reasonable?
This seems reasonable to me.
I really think that this project should have a clear guideline for adding new extensions
I agree, but this code base is in maintenance mode for the most part. There is no one actively working on this project right now. 😢
So an invalid glTF file that was previously okay to load would fail to load.
If this is true, I would think this is an oversight. We may have gotten lucky that it didn't introduce problems.
it looks like this SDK doesn't act on extensionsRequired contents so that's not a problem (although that is in stark contrast with other validation decisions made by this SDK).
That does seem odd. I can't speak to the decisions on this. I took over this project from another team.
from gltf-sdk.
Ok thanks! This is currently low on my priority list but the course of action is now clear :)
from gltf-sdk.
Related Issues (20)
- Is it possible to write not interleaved buffers? HOT 1
- Exceptions necessary ? HOT 2
- Can it export quads into gltf ? HOT 2
- How to export data as ascii text in GLTF? HOT 1
- ByteOffset should be a multiple of 4 in Javascript HOT 3
- Changes - suggestion for the Serialization example HOT 4
- Procedure to upgrade for the support many Vertex Colors ? HOT 2
- Example for animation? HOT 3
- Buffer error when reaching 2GB (2.147.869.560) byteLength HOT 18
- about draco HOT 2
- clang/LLVM for Windows build failures HOT 3
- compile error, SchemaValidation HOT 1
- This repo is missing important files HOT 1
- GenerateSchemaJsonHeader.ps1 fails because D:\source\GLTFSDK does not exist
- Point size and Line width HOT 1
- googletest, POWERSHELL_PATH-NOTFOUND and other errors on Linux HOT 1
- Request for GLTF-SDK Support on Visual Studio 2022 HOT 2
- Add `ResourceWriter::Write` overload with a buffer id parameter instead of a buffer view
- Can a single vertex have multiple UV coordinates depending on how many faces it belongs to? HOT 1
- Add support for punctual lights extension
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gltf-sdk.