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.

Sign up for GitHub

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

Jump to bottom

Performance improvements for tracksViewChanges #2487

Merged
merged 7 commits into from
Sep 21, 2018
Merged

Performance improvements for tracksViewChanges #2487

merged 7 commits into from
Sep 21, 2018

Conversation

danielgindi
Copy link
Contributor

Does any other open PR do the same thing?

No

What issue is this PR fixing?

Performance improvements for #2477 #1870

How did you test this PR?

Full project

@danielgindi
Copy link
Contributor Author

@rborn some improvements related to the latest issue with tracksViewChanges and custom Image tags in markers. Should be merged in before a release to avoid a too heavy toll on users with tracksViewChanges as default.

@rborn
Copy link
Collaborator

rborn commented Sep 13, 2018

@alvelig could you have a look 🐽 I'm heading to ReactAlicante so I won't be able to do anything until Monday

@alvelig
Copy link
Contributor

alvelig commented Sep 13, 2018

@danielgindi Do I get it right, that it comes as a replacement for tracksViewChanges? I think that we should enable this behavior by default.

@tmclemons
Copy link

This redraw update, will this also help with the memory leaks issues related to maps rerendering?

@danielgindi
Copy link
Contributor Author

There's a bunch of stuff in here:
redraw should be used when you know that you can/wanna control when the view's screenshot is taken for the marker.
tracksViewChanges is easier, and is the default behavior that google designed (on iOS) for allowing animations etc. It is just doing a lot more that needed in many cases.

I think this should be left up to the user and the use case.

The tracksViewChanges feature itself will consume less CPU and memory now, as what happened in the original repo version is that createDrawable() created a new bitmap every time. This is both time consuming and memory consuming. I made it reuse the bitmap as long as it didn't resize.
So there is a significant performance improvement on that end too.

@danielgindi
Copy link
Contributor Author

Also originally I tried to check for isDirty() on the view, but specifically the Drawee from Fresco does not propagate the dirty flag to parents. It's waiting for a draw request to actually test whether it should draw (or mark itself as dirty to begin with). So it's kind of an infinite logic loop that you can't escape.

@alvelig
Copy link
Contributor

alvelig commented Sep 13, 2018

Excuse me, but why do we need tracksViewChanges? I missed that one

@tmclemons
Copy link

@danielgindi awesome i cant wait till this update is pushed to the repo

@danielgindi
Copy link
Contributor Author

@alvelig tracksViewChanges as the name implies - tracks the view's changes and updates the markers accordingly. This basically means that you could use animations in the markers.

@alvelig
Copy link
Contributor

alvelig commented Sep 14, 2018

So by default tracksViewChanges is not required. That's why you introduced redraw, if it is required to redraw a marker once. Now I see. By default this shouldn't be required, right?

@danielgindi
Copy link
Contributor Author

Well it depends :)
I have some custom markers that have animations (kind of progress bars) whete tracksViewChanges is required, and some where a simple redraw call would suffice.

The thing is that even with iOS until now the way to ensure that a custom marker will be drawn is to keep the default tracksViewChanges: true, and set it ti false on componentDidUpdate for performance. This is because RN rendering is async and you first take a frame of an empty view.

So by default it is the correct mechanism, but then the user should disable it on the correct event like componentDidUpdate or Image’s onLoad or other custom events.

It’s even more complex than that, as you might have a custom marker that has an image AND other views.
So first frame takes an empty image. After first render happened, the trackViewChanges will take an image of the actual view, and after loading the image it will update to the marker with the image loaded- only then you want to disable tracking.

So my main recommendation is adding docs and more docs :)

@danielgindi
Copy link
Contributor Author

We could also wrap the marker component with another, that will have built in logic for componentDidUpdate for the simple custom markers - but that would be a breaking change.

@alvelig
Copy link
Contributor

alvelig commented Sep 15, 2018

@danielgindi why would it be a breaking change?

@danielgindi
Copy link
Contributor Author

As people expect it now to be true by default (as it has always been in iOS), and be disable when they set it to disabled.

@alvelig
Copy link
Contributor

alvelig commented Sep 15, 2018

Great, thanks for the explanations. I agree!

@rborn rborn merged commit 78a36ac into react-native-maps:master Sep 21, 2018
@danielgindi danielgindi deleted the feature/tracking_perf branch September 23, 2018 12:59
ryanbourneuk pushed a commit to 3sidedcube/react-native-maps that referenced this pull request Dec 10, 2018
Merge remote-tracking branch 'upstream/master'
5f21682 
* upstream/master: (28 commits)
  Calculate bounding box from region (react-native-maps#2615)
  [iOS GoogleMap] Fix animateCamera (react-native-maps#2608)
  Fix type definition error (react-native-maps#2607)
  [Android] Fix app crash in Android if building found but cannot getActiveLevelIndex (react-native-maps#2598)
  Provide a camera system (react-native-maps#2563)
  Get visible map bounding box (react-native-maps#2571)
  [0.22.1] Release (react-native-maps#2574)
  Move dev only deps to devDependencies. (react-native-maps#2548)
  Specify how to use Google Maps (react-native-maps#2550)
  r2507: remove marker: Attempt to invoke virtual method 'void com.google.android.gms.maps.model.setIcon(com.google.android.gms.maps.model.BitmapDescription)' on a null object reference #: remove marker: Attempt to invoke virtual method 'void com.google.android.gms.maps.model.setIcon(com.google.android.gms.maps.model.BitmapDescription)' on a null object reference (react-native-maps#2555)
  update to clarify cacheEnabled is apple maps only
  [0.22.0] Release (react-native-maps#2535)
  Fix for “The specified child already has a parent”
  Improve installation docs (react-native-maps#2541)
  fix fitToSuppliedMarkers function (react-native-maps#2524)
  Performance improvements for tracksViewChanges (react-native-maps#2487)
  fix spelling mistakes
  Added flag to make sure that there has an Observer of view.
  hotfix PR react-native-maps#2478
  Fix a peer dependencies warning
  ...
@slorber
Copy link
Contributor

slorber commented Dec 12, 2018

hey @danielgindi wondering if you have any idea how tracksViewChange could play well with native driver animations?

See #1705 (comment)

Stalder referenced this pull request in 91team/react-native-maps Feb 5, 2019
@Stalder
Performance improvements for tracksViewChanges [https://github.com/re…
3a1736f 
…act-native-community/react-native-maps/pull/2487](https://github.com/react-native-community/react-native-maps/pull/2487)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@danielgindi @rborn @alvelig @tmclemons @slorber