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

Add GLTF Scene Loader #425

Merged
merged 3 commits into from
Jun 7, 2024
Merged

Add GLTF Scene Loader #425

merged 3 commits into from
Jun 7, 2024

Conversation

anishmgoyal
Copy link
Collaborator

@anishmgoyal anishmgoyal commented Feb 13, 2024

Using the library cgltf, implements a GLTF scene laoder for BigWheels's scene graph.

Also, defines an interface for generic scene loaders, in case we come up with other ways of storing and loading scenes.

@apazylbe apazylbe requested review from Keenuts and removed request for chaoticbob March 18, 2024 17:00
src/ppx/scene/scene_gltf_loader.cpp Show resolved Hide resolved
src/ppx/scene/scene_gltf_loader.cpp Show resolved Hide resolved
src/ppx/scene/scene_gltf_loader.cpp Outdated Show resolved Hide resolved
src/ppx/scene/scene_gltf_loader.cpp Outdated Show resolved Hide resolved
src/ppx/scene/scene_gltf_loader.cpp Outdated Show resolved Hide resolved
src/ppx/scene/scene_gltf_loader.cpp Show resolved Hide resolved
@Keenuts
Copy link
Member

Keenuts commented Mar 26, 2024

Just realized I never clicked "send review"

@Keenuts
Copy link
Member

Keenuts commented Mar 26, 2024

This code now being orphaned, and quite self contained (we don't break bigwheels by bringing it in), I would be fine to merge this as-is (minus the small fixes), and iterate from there, as long as this compiled on all 3 platforms.

@anishmgoyal anishmgoyal force-pushed the addSceneLoader branch 2 times, most recently from 0afb2ee to c685cdc Compare April 9, 2024 16:36
@anishmgoyal
Copy link
Collaborator Author

This code now being orphaned, and quite self contained (we don't break bigwheels by bringing it in), I would be fine to merge this as-is (minus the small fixes), and iterate from there, as long as this compiled on all 3 platforms.

@Keenuts ehh - I don't mind trying to address the comments directly in this PR, that way nothing slips through unaddressed. I don't think this PR is needed too immediately

@anishmgoyal
Copy link
Collaborator Author

@Keenuts can you re-review discussions above?

Copy link
Collaborator

@apazylbe apazylbe left a comment

Choose a reason for hiding this comment

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

I have to admit I mostly skimmed the code at the end. I left some notes (some mainly for the record, feel free to ignore or add your thoughts).

include/ppx/scene/scene_gltf_loader.h Show resolved Hide resolved
include/ppx/scene/scene_gltf_loader.h Show resolved Hide resolved
src/ppx/scene/scene_gltf_loader.cpp Show resolved Hide resolved
src/ppx/scene/scene_gltf_loader.cpp Show resolved Hide resolved
@Keenuts
Copy link
Member

Keenuts commented May 13, 2024

This code now being orphaned, and quite self contained (we don't break bigwheels by bringing it in), I would be fine to merge this as-is (minus the small fixes), and iterate from there, as long as this compiled on all 3 platforms.

@Keenuts ehh - I don't mind trying to address the comments directly in this PR, that way nothing slips through unaddressed. I don't think this PR is needed too immediately

Sure, we can address the open comments. But since this is a 4K Loc PR adding a new API with 0 tests, I think we'll miss many bugs anyway. So as long as nothing relies on this before it's proven to work, I'm OK merging this.

@apazylbe
Copy link
Collaborator

@Keenuts @anishmgoyal any objections to merging this as is and addressing the outstanding comments in a follow up?
This way @footballhead will be able to make progress on GLTF work

anishmgoyal and others added 3 commits June 7, 2024 14:09
Co-authored-by: Hai Nguyen <chaoticbob@users.noreply.github.com>
Co-authored-by: Aliya Pazylbekova <apazylbe@users.noreply.github.com>
Also: add a default destructor to PrimitiveBatch
@anishmgoyal
Copy link
Collaborator Author

@Keenuts @anishmgoyal any objections to merging this as is and addressing the outstanding comments in a follow up? This way @footballhead will be able to make progress on GLTF work

Sorry - I've been distracted with other things. Merging this sounds good to me

@apazylbe apazylbe merged commit 78e19af into google:main Jun 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants