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

Bug fixes and new features #29

Merged
merged 28 commits into from
May 4, 2020
Merged

Conversation

cristianoccazinsp
Copy link

@cristianoccazinsp cristianoccazinsp commented Apr 15, 2020

Hello,

First of all, thanks for the amazing work!

I've been updating the source code of this library to fit my project's needs, and I thought it would be nice to share this work back with a PR. Please note that there are a few breaking changes, so you may want to review this carefully and/or update it briefly.

Summary (non breaking changes):

  • Included some bug fixes from Fix motion tracking on iOS #26 (tracking on iOS)
  • Implemented local files support. imageUrl now supports a local file://path or /file/path file (also fixes bridge.bridgeLoader is deprecated and will not work in newer versions of RN #23)
  • Various Android and iOS bug fixes. Mostly related to bridge handling, cancelation of downloads, and life cycles.
  • Added a new event to fire when the image has been downloaded. This solves an issue when you hide the view (e.g., with display 'none') in order to show a loading indicator and want to update the state once the download finishes. Currently, Android will not fire any event if the view is hidden. With this change, it will now fire and work as expected.
  • CTPanoramaView now also supports finger rotation
  • Migrated example to RN >= 0.60 which was a rather painful migration (androidx and stuff)
  • Readme updates. Added some extra instructions related to Swift issues

Summary (breaking):

  • Bump to Swift 5
  • For some reason I had to remove the @lightbasenl from the package name for it to work properly and make development easier (no build step). This made me remove the .ts files and build steps so the package can be run right away. Note that I'm still using a type definition for autocomplete/docs.
  • on iOS, it was not possible to use both touch and sensor rotation, and in fact, using enableTouchTracking={true} disabled motion tracking (as opposed to Android). With this change, both motion and touch can be enabled, although once touching starts, sensor tracking gets disabled. In order to do this, I had to also fork the CTPanoramaView library. This means that until CTPanoramaView merges the PR (which has some improvements), the dependency has to be added on the user's podfile as described on the readme (pod 'CTPanoramaView', :git => 'https://github.com/cristianoccazinsp/CTPanoramaView.git', :branch => 'control-both'). CTPanoramaView doesn't seem maintained, so perhaps it's a better idea to include it on this repo, but I wasn't able to compile both swift and obj-c together like this.

TODOs:

  • Bring back the @lightbasenl namespace or change the package's npm location to just be react-native-panorama

  • Vendor CTPanoramaView either by adding it to the source code or creating a new pod repo so we don't need to wait for the author merging new changes; the repo seems quite dead honestly.

  • Continue to improve CTPanoramaView to support simultaneous touch and sensor rotation (or update Android version to have the same behaviour as iOS)

  • GVRSDK is deprecated, it needs to be replaced with the new cardboard SDK on Android

  • Allow Android to disable sensor rotation and only use touch-based controls (like iOS). Once this is done, enableTouchTracking should be changed to be an option to pick between "both", "touch", "sensor" so all 3 options can be used.

Lastly, I might be adding a few more improvements, but wanted to share these changes early :)

@cristianoccazinsp
Copy link
Author

For iOS, now both finger and sensor rotation is supported at the same time, bringing it to par with Android: scihant/CTPanoramaView#47

@rodymolenaar rodymolenaar changed the base branch from master to next May 4, 2020 08:21
@rodymolenaar rodymolenaar self-requested a review May 4, 2020 08:27
@rodymolenaar
Copy link
Member

@cristianocca I'll be merging this into a branch here, so I can continue with incorporating these changes.

@rodymolenaar rodymolenaar merged commit 991f7e3 into lightbasenl:next May 4, 2020
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.

bridge.bridgeLoader is deprecated and will not work in newer versions of RN
4 participants