Animation refactor #377
Animation refactor #377
Conversation
Half a map added to the west and half a map added to the east. Working exactly as it should, but code is not pretty.
|
Okay. I think I'm done with this for the moment (and need to shift gears for the next couple days). @colinmegill: I tried to clean up code where possible, things like:
Biggest functional change is duplicating map with adding a half map to the west and a half map to the east, rather than the full triplicating. This improves performance and provides a more natural set of bounds. This is just FYI for @colinmegill, but @jameshadfield could you do a quick review on Monday and merge if it looks good. |
|
Looks good to me - i've made some comments in issue #367 Will let @colinmegill take a look, but I plan to merge later this afternoon (since animation is currently disabled in master anyway) |
|
Ok, will take a look now!
…On Mon, Jul 10, 2017 at 10:27 AM james hadfield ***@***.***> wrote:
Looks good to me - i've made some comments in issue #367
<#367>
Will let @colinmegill <https://github.com/colinmegill> take a look, but I
plan to merge later this afternoon (since animation is currently disabled
in master anyway)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#377 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABsDGVTlL-85Pl52xoysht34AQ194Fmlks5sMl8VgaJpZM4OR1Hd>
.
|
|
Great. Thanks both. |
| @@ -488,38 +497,6 @@ class Map extends React.Component { | |||
| } | |||
| }, animationTick); | |||
|
|
|||
| // controls: state.controls, | |||
There was a problem hiding this comment.
This comment can stay, I will clean up on the requestAnimationFrame pass
| @@ -25,18 +24,16 @@ import { | |||
| // datasetGuid: state.tree.datasetGuid, | |||
| treeVersion: state.tree.version, | |||
| treeLoaded: state.tree.loaded, | |||
| controls: state.controls, | |||
| splitTreeAndMap: state.controls.splitTreeAndMap, | |||
| location: key, // Thailand: | ||
| total: value.length, // 20, this is an array of all demes of a certain type | ||
| color: averageColors(value), | ||
| coords: leafletLatLongToLayerPoint(lat, long, map) |
There was a problem hiding this comment.
This (apparently, but I could be wrong here) no longer relies on new L.LatLng, is that intentional?
There was a problem hiding this comment.
I did this to reduce code duplication. leafletLatLongToLayerPoint should be handing back a new L.LatLng(lat, long) in the exact same fashion as before, just shelved out into a function.
|
All set - this is looking great |
|
These changes look good James. |
Progress on #367
Before this branch (current
master) if getting the following performance for Ebola:getLatLongsWith this branch in place I'm getting:
getLatLongsupdateVisibilityBefore this branch (current
master) if getting the following performance for 3y flu region resolution:getLatLongsWith this branch in place I'm getting:
getLatLongsupdateVisibilityGoing to keep updating these numbers as I work on the PR. Serving to help me remember if there's performance increases.