-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
glTF 2.0 support #1904
glTF 2.0 support #1904
Conversation
Regarding the naming of container - no resource type in the asset system is named after its file format. We have "texture" resources and "model" resources and "animation" resources and suddenly naming this "glb" will break convention. It's also slightly incorrect because the handler supports both GLB and GLTF format (which are related). So for now I will rename these to GlbResource and GlbHandler as the lesser of two evils, but I do so under protest, until someone comes up with a better name! |
Pretty much any name can be aliased in the future. So it's not like we can't adjust these decisions in the future. But I would go along with @slimbuck and @mvaligursky here and say that names like |
Reason for load and open, is that developer might provide raw data from somewhere else, instead of loading. |
* @example | ||
* app.assets.loadFromUrl("../path/to/texture.jpg", "texture", function (err, asset) { | ||
* var texture = asset.resource; | ||
* }); | ||
*/ | ||
loadFromUrl: function (url, type, callback) { | ||
loadFromUrl: function (url, type, callback, filename) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is polluting the API unnecessarily. Callbacks are expected to be the last argument and since it would be a breaking change to move filename before callback it's not really viable either. How about adding a new method in the pc.Asset
class for setting the name instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow, Asset already exposes name and filename? This was added so the GlbParser can identify whether the data is GLB or GLTF when loading from a blob.
…s and asynchronous parsing of glb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a bunch of comments from a first pass review of this. Looks great - so exciting!
So we don't have to use playcanvas-gltf anymore? |
Hi @Velbi, It depends. The engine GLTF implementation is missing morph target support and draco compression. These haven't been added yet but are planned. Also the animation system is new and might not be as fully featured yet as the one in playcanvas-gltf. Thanks! |
* stable: (48 commits) [RELEASE] v1.26.0 [FIX] null access when pushing SpectorJS marker without camera (playcanvas#1933) [FIX] pc.XrHitTest when WebXR not available Update package-lock.json Downgrade mocha to 6.2.2 WebXR AR hit test support (playcanvas#1926) [ImgBot] Optimize images (playcanvas#1927) [DOCS] Make anim API private (playcanvas#1924) Update NPM dependencies. [FIX] Update some example paths Update to latest JSDoc template [FIX] Updating of particle systems local bounds Add pc.drawTexture to render texture to target using a shader (playcanvas#1920) Optimize Grab-Pass for WebGL 2 and fix when using anti-aliased RenderTarget (playcanvas#1918) [DOCS] Document drawQuadWithShader and copyRenderTarget (playcanvas#1919) [FIX] Mouse wheel events on elements (playcanvas#1916) glTF 2.0 support (playcanvas#1904) Update to latest JSDoc template. Add support for ES6 script classes (playcanvas#1908) Particle system randomize sprite animations (playcanvas#1911) ...
This PR contains the following:
I confirm I have signed the Contributor License Agreement.