-
Notifications
You must be signed in to change notification settings - Fork 92
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
Upgrade To THREE 0.166.0, upgrade to Expo SDK 51 #301
Conversation
Great work guys! Let's get this merged please @keith-kurak |
thanks |
This is great stuff! Would love to see it merged. |
Any updates on this? This really needs to be merged, |
Ooof, I really need this too. |
(this was automatically done by yarn)
Should Fix expo#164
Any updates on what is required for this to be merged? |
@markrickert I have tested your PR to address #297 package.json "expo-three": "github:markrickert/expo-three.git#dev", It does not seem to resolve the error: Failed to getSize of file:///var/mobile/Containers/Data/Application/... I can confirm that downgrading to Expo 49 fixes the issue |
I am having some problems installing the dev branch.
It seems like the |
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.
Hey there!
First of all, I'm sorry that this PR has been left alone 😅 I think this happened mostly due to a large scope of changes, including ones that are probably unnecessary and/or could be split into smaller pieces so it's easier for us to review it.
I did the first pass of review, mostly just questions so I can better understand the motivation behind those changes.
I really appreciate the time and effort you guys took to make this!
@tsapeta Thanks for the review. I'm new to working on expo modules and i'm just trying to push this library forward. I've addressed your comments in a new commit and i'll comment or resolve them here on github too. |
How is this looking? Could really use it! |
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.
Looking good now, thank you! ❤️
It's been published in version |
It is working for me. 👍🎉
|
@tsapeta I just checked in a new app, adding Thanks for getting this over the finish line! 🥳 |
This PR is the culmination of a few weeks of work and experimentation around the
expo-three
andthree.js
ecosystems. @Jpoliachik and I were able to get many more examples running in the example app and lots of upgrades to bring this library back into line with the latest expo standards and SDK.Here are the updates in no particular order:
expo-three
instead of the build directory, making developing on the example app much easier without the build service running.expo-three
to use the latestexpo-module-scripts
expo-three-orbit-controls
(had to patch because it still relies on an old version of THREE).The example app works on android and web, but I haven't tested on iOS because the simulator doesn't support web-gl.
All tests pass (including the new ones!)
Linter passes
Please let me know what else needs to be done to get this merged!
This PR will Close: #164, #199, #212, #238, #270, #287, #297, #298, & #299
Thanks to @Jpoliachik for his help in getting this branch working and for porting over some THREE web examples over from the web examples.