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

feat(GLTFImporter): add GLTFImporter #2479

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Conversation

daker
Copy link
Collaborator

@daker daker commented Jun 14, 2022

Context

This PR adds initial support for GLTF 2.0 format

Results

  • What versions of glTF format should we support? (both or just 2.0 ?) => only 2.0
  • Load glTF (.gltf) with separate resources: .bin (geometry, animation, skins) and .jpg or .png image files
  • Load glTF (.gltf) with embedded resources (as Data URIs)
  • Load glTF geometry compressed with the Draco library
  • Load binary glTF (.glb) using the binary container format
  • Handle animations loading
  • Handle skins loading
  • Handle lights loading
  • ORM Texture support with OffscreenCanvas (GLSL support [Feature] Add for support for combined ORM Texture #2961)
  • Extensions support? what is the minimal set of extensions to support?
    • KHR_draco_mesh_compression
    • KHR_lights_punctual
    • KHR_materials_unlit
    • KHR_materials_ior
    • KHR_materials_specular
    • KHR_materials_variants
    • EXT_texture_webp
    • EXT_texture_avif

Changes

  • Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js:
    • OS:
    • Browser:

Fixes: #210

@daker

This comment was marked as resolved.

@supriome
Copy link

supriome commented Jul 7, 2022

Hi @daker , Any progress there?

@daker

This comment was marked as resolved.

@jourdain
Copy link
Collaborator

Hi @daker,

I may start looking into it. Do you have a feel of how much more work needs to go into it?
Does your code mostly work but fails in some use cases? Basically I just want to have a feel on what kind of help you are looking for knowing that I'm looking for a glb loader...

Do you think we can reformat/rebase and merge it in its current state, or would it be better to work on the same branch until X and Y are solved? If so, knowing what X and Y are would be great.

Thanks for your hard work pushing vtk.js forward,

Seb

@daker
Copy link
Collaborator Author

daker commented Sep 14, 2022

I have updated the description with a list of questions/features to be addressed. Basically the loader is able to parse the gltf with simple embedded resources like the box or colored box.

As you can see on the images, there is something missing in the code to render those boxes correctly, @finetjul told me that faces/cells do not seem to be made of the right point ids, i am not good on this part.

#210 (comment)

My idea is to have a simple SceneLoader that will act like this :
Scene[$sceneId] -> Nodes -> Node -> Actors -> Actor (load texture and apply materials)-> Add the actor to the renderer

I would suggest to work on a branch, once we have a basic SceneLoader with basic parsing and rendering then we can merge and iterate over with adding animations and extensions depending on what we have decided.

@jourdain
Copy link
Collaborator

Sounds good, thanks for the update.

@daker
Copy link
Collaborator Author

daker commented Dec 4, 2023

@jourdain I am back working on this, i made some very good progress, no animation, lights , cameras, skins, morph or extensions so far

The ORM textures are slow to display since they are computed(using canvas) by extracting the channels from a the combined texture, this should be done by the GPU like in vtk c++ by providing a setORMTexture (FYI @martinken)

screen-capture.2.webm

@jourdain
Copy link
Collaborator

jourdain commented Dec 4, 2023

@daker daker force-pushed the gltf-reader branch 3 times, most recently from 0d01463 to 597eaae Compare September 27, 2024 15:53
@daker
Copy link
Collaborator Author

daker commented Sep 27, 2024

@jourdain Pushed new update

screen-capture.1.webm

Animations are not working well on webGL, not sure if it's my code or vtk.js and it's not working with webGPU at all, the lags are due to the recording.

screen-capture.2.webm

@jourdain
Copy link
Collaborator

I may need to read it more carefully but it looks great! Should we try to get it merged or is it still in WIP?

@daker
Copy link
Collaborator Author

daker commented Sep 27, 2024

Yes we can merge it, I have added support for GLB and Draco loading using vtkDracoReader

@daker daker changed the title WIP: add GTLFReader feat(GLTFReader): add GLTFReader Sep 27, 2024
@daker daker marked this pull request as ready for review September 28, 2024 00:01
@jourdain
Copy link
Collaborator

Wow, I read it all and it is not small! Good job and thanks for your contribution.

@jourdain
Copy link
Collaborator

@finetjul @floryst any feedback or can we merge it?

@finetjul
Copy link
Member

I'll have a look

@daker
Copy link
Collaborator Author

daker commented Sep 28, 2024

i will address the comments and fix the GLB buffer loading

Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your great contribution.

Here are some various comments.

  • You may want to expose at least an high-level .d.ts file.
  • Can you please add an entry in the example Gallery (Documentation/content/docs/example/index.md)?

Sources/IO/Geometry/GLTFReader/Animation/AnimationMixer.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Animation/AnimationMixer.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Animation/AnimationTrack.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Animation/AnimationTrack.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Animation/AnimationTrack.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Parser.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Parser.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Parser.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Contants.js Outdated Show resolved Hide resolved
Sources/IO/Geometry/GLTFReader/Reader.js Outdated Show resolved Hide resolved
Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daker daker force-pushed the gltf-reader branch 2 times, most recently from 6938b2d to 8cac623 Compare October 11, 2024 10:22
@daker
Copy link
Collaborator Author

daker commented Oct 11, 2024

It's ready to be merged,

@daker daker force-pushed the gltf-reader branch 2 times, most recently from d3275c1 to bf8c870 Compare October 11, 2024 10:47
Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daker daker force-pushed the gltf-reader branch 2 times, most recently from b496d08 to d9914fe Compare October 12, 2024 15:39
@daker daker force-pushed the gltf-reader branch 2 times, most recently from 2cda0ce to 4073c2e Compare October 21, 2024 22:28
@daker
Copy link
Collaborator Author

daker commented Oct 21, 2024

@floryst i have rebased this branch also in case you want to merge it

@daker
Copy link
Collaborator Author

daker commented Nov 8, 2024

@floryst when do you plan to merge the PR ?

@floryst
Copy link
Collaborator

floryst commented Nov 11, 2024

@daker sorry, I missed this because of the default github PR sort order. I'll fix the conflict and get this merged.

@floryst floryst added this pull request to the merge queue Nov 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 11, 2024
@daker
Copy link
Collaborator Author

daker commented Nov 13, 2024

Not sure why it was removed from the queue while it has not conflicts

@floryst
Copy link
Collaborator

floryst commented Nov 13, 2024

I'll try again.

@floryst floryst added this pull request to the merge queue Nov 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 13, 2024
@floryst
Copy link
Collaborator

floryst commented Nov 13, 2024

@daker can you rebase on master?

@floryst floryst added this pull request to the merge queue Nov 13, 2024
Merged via the queue into Kitware:master with commit 0c3933d Nov 13, 2024
3 checks passed
@daker daker deleted the gltf-reader branch November 13, 2024 17:15
Copy link

🎉 This PR is included in version 32.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Automated label label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Automated label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glTF support
5 participants