Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Vendor CTPanoramaView #43

Closed

Conversation

cristianoccazinsp
Copy link

@rodymolenaar I'm not sure if you're still interested in merging these changes to support finger + orientation rotation (#29).

However, I'm uploading a new PR which vendors the CTPanoramaView into this same library to make development easier since that repo seems dead / not willing to accept new changes.

No new changes (from the original PR), except a bump to iOS 10 (to match RN updates) and config updates to support swift + objc in the same project.

@rodymolenaar
Copy link
Member

This is looking great, at the moment I won't be able to test it though. But if you're feeling confident I'd be willing to merge it!

@cristianoccazinsp
Copy link
Author

It still has some of the breaking changes I mentioned in the other PR:

  • Bump to Swift 5
  • Removed the @lightbasenl namespace from imports
  • Now requires iOS 10 (like RN)

So there are these breaking changes, I wouldn't like to get blamed for these if other people are using the library. Having said that, I've been using the branch for a while and it works great.

@tcorreiaubi
Copy link

Hey, I'm using @cristianoccazinsp branch and so far everything seems to be working properly, I will also validate on iOS once possible, but on android had zero problems with it

@mapsi-fixi
Copy link

I can confirm this branch is working on iOS and RN 0.63.2.
Current master is useless with new RN versions - working on debug mode (iOS), but it is impossible to build a release because of old bridge implementation. We need this PR :)

@cristianoccazinsp
Copy link
Author

Hey guys, there's one more "improvement" I have pending on this branch, which is basically moving everything to Swift (and get rid of the intermediary objective-c View and modules). This makes the component more readable and gets rid of some import issues (that forces you to clean and build) due to swift headers. But I've been working on another swift module so this is a big todo for me.

@captdeys
Copy link

Hey @cristianoccazinsp, this maybe wrong thread but since you were talking about improvements, would you have an idea why RAM usage is high when using this module? Are 360 images very ram intensive? Like I was seeing a 250MB jump in RAM.

@cristianoccazinsp
Copy link
Author

@adnansyedimadi I can't really tell. I guess the whole Scene kit and sensors stuff may explain the memory spikes. Also 360 images being decoded and loaded into memory are much bigger at the end, and I don't think there's any bitmap resizing / optimization in place to shrink images to the screen resolution, so that also can add significant work to the device.

@rodymolenaar
Copy link
Member

Thanks for your great work @cristianoccazinsp

I've merged these changes into master and released @lightbase/[email protected] to npm.

I also took the liberty to invite you as a maintainer since you're already so involved with the library.

@cristianoccazinsp
Copy link
Author

cristianoccazinsp commented Feb 23, 2021

@rodymolenaar thanks for the merge! I'm trying to use this new merge as opposed to my own branch, but looks like there are some issues. I am getting [!] No podspec found for @lightbase in ../node_modules/@lightbase/react-native-panorama-view when running pod install.

Update: Looks like just a bug with how podspec name and actual file name. I will see about fixing this in another branch together with a full migration to Swift.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants