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

Added features to load models from .gltf and .glb files. #211

Merged
merged 5 commits into from
Oct 21, 2023

Conversation

Jaemin-VIRNECT
Copy link
Contributor

Added features to load models from .gltf and .glb files. (using GLTFSceneKit)

And examples also added.

@olexale
Copy link
Owner

olexale commented Oct 12, 2023

Hello @Jaemin-VIRNECT,

Thank you so much for this PR! I'll review it on the weekend and if everything is good publish a new version. Meanwhile, could you please make sure you are not using "print" in the production code and that all files are formatted? Was there a reason to bump the minimum SDK version?

Thanks again!

Kind regards,
Oleksandr

@Jaemin-VIRNECT
Copy link
Contributor Author

Jaemin-VIRNECT commented Oct 12, 2023

Hello @olexale ,
Thank you for your quick reply and great work.
All dart files have their code formatted according to Android Studio default settings.

I checked my PR again and it seems that my NodeBuilder.swift is using the print().
After a quick review of your code it seems I should have used logPluginError().

If there is a problem with the file format, etc., would you please change it?
(I am a beginner in Swift. :( )

And about the minimum SDK version,
Enum.name requires a minimum SDK version >= 2.15.0 (Android Studio warning).

Copy link
Owner

@olexale olexale left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR! I bet many engineers would love to see this functionality. My initial idea was to keep only pure ARKit functionality without any 3rd-party SDKs, but I can admit that many devs would love to be able to use .glb and .gltf files in their projects, so let's add it.

I've added a few comments and propositions to the PR. Please have a look.
I might be able to help you with PR next weekend if you do not make changes before that.

Thanks again for your efforts!

README.md Outdated Show resolved Hide resolved
example/lib/load_gltf_or_glb_page.dart Outdated Show resolved Hide resolved
example/lib/main.dart Outdated Show resolved Hide resolved
ios/Classes/NodeBuilder.swift Outdated Show resolved Hide resolved
lib/src/arkit_reference_node.dart Outdated Show resolved Hide resolved
lib/src/arkit_reference_node.dart Outdated Show resolved Hide resolved
@olexale olexale merged commit 9c2ceaa into olexale:master Oct 21, 2023
@olexale
Copy link
Owner

olexale commented Oct 21, 2023

Thanks again, @Jaemin-VIRNECT! I'll make some clean up (unrelated to your changes) in the plugin and release a new version with your contribution soon!

@olexale
Copy link
Owner

olexale commented Oct 22, 2023

I just pushed version 1.0.7 with your changes. Thanks!

@Jaemin-VIRNECT
Copy link
Contributor Author

Thank you so much! @olexale

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.

2 participants