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.

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

Migrate the main map to Deck.gl #773

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

tahini
Copy link
Collaborator

@tahini tahini commented Nov 14, 2023

Step by step migration to the Deck.gl library for maps.

@tahini
Copy link
Collaborator Author

tahini commented Nov 14, 2023

Will fix #552

@davidmurray If you want to work on some tasks related to this issue, let me know and you can do PR on https://github.com/tahini/transition/tree/deck-gl-pow branch to get it in this PR

@tahini
Copy link
Collaborator Author

tahini commented Nov 30, 2023

@davidmurray Je suis à faire fonctionner les événements pour créer un nouveau path, et on dirait que ça construit le path à l'envers. Est-moi de mon côté ou serait-il possible que les flèches du path animé soient inversées? Car l'array se fait dans la direction à laquelle je m'attends (nouveau noeud en arrière), mais l'animation me montre l'autre direction.

@tahini
Copy link
Collaborator Author

tahini commented Nov 30, 2023

@davidmurray Pas besoin de fixer le problème, juste me dire si tu es certain de la direction des flèches, sinon, je vais regarder de mon côté.

@davidmurray
Copy link
Collaborator

@tahini Ça se peut, je ne me souviens plus si c'était dans la bonne direction. Quand tu prends le même path (pré deck.gl et post deck.gl), l'animation change de sens?

Le code que j'ai push provient à 80% de https://github.com/visgl/deck.gl/tree/master/modules/extensions/src. Ça me surprendrait qu'ils aient fait une erreur.

Mais si c'est le cas on pourrait peut-être juste ajouter un *-1 ici https://github.com/tahini/transition/blob/deck-gl-pow/packages/transition-frontend/src/components/map/layers/AnimatedArrowPathLayer.tsx#L122

@tahini
Copy link
Collaborator Author

tahini commented Dec 1, 2023

Je confirme que le path est à l'envers (et que les nodes et waypoints ne sont pas encore avec le bon look :p )

Capture d’écran du 2023-12-01 10-35-41

Capture d’écran du 2023-12-01 10-44-06

@tahini
Copy link
Collaborator Author

tahini commented Feb 6, 2024

@kaligrafy @greenscientist ready for review, see the #552 issue for what was done and what would be left to do as separate and later issues. I think, the minimial viable product for deck.gl support is there. Please test it if you can to make sure I didn't miss anything important.

@@ -265,11 +265,17 @@
"General": "Préférences générales",
"DefaultSection": "Section par défaut lors de l'ouverture",
"InfoPanelPosition": "Position du panneau d'information",
"MapStyle": "Fond de carte",
"DefaultColor": "Couleur par défaut",
"DefaultWalkingSpeedKph": "Vitesse de marche par défaut (km/h)",
"DefaultWalkingSpeedKphHelp": "La vitesse de marche varie selon l'âge et le genre entre 3 km/h (personnes âges et jeunes enfants) et 7 km/h (personnes jeunes et/ou très actives). Les hommes sont en moyenne un peu plus rapides que les femmes et de manière générale, la vitesse de marche diminue avec l'âge. La vitesse d'une personne en chaise roulante manuelle varie entre 2 et 3 km/h selon la force physique.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personnes âges -> personnes âgées

Copy link
Collaborator

@greenscientist greenscientist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have much comments, a few small things about commented code.

I imagine we have tested the custom tile that we are using in prod?

@@ -74,37 +74,37 @@ class Toolbar extends React.Component<LayoutSectionProps, TransitionToolbarState
}

onUpdateLayers = () => {
const layersVisibility = {};
/* const layersVisibility = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want to keep this commented code?? If so it would need a comment.

@@ -100,106 +149,48 @@ const onNodeSectionMapClick = (e: MapboxGL.MapMouseEvent) => {
);
newTransitNode.startEditing();
serviceLocator.selectedObjectsManager.select('node', newTransitNode);
serviceLocator.eventManager.emit('selected.updateAutocompleteNameChoices.node', getRoadLabelAround(map, e));
// FIXME Migration to DeckGL: reimplement this
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is those FIXME/TODO documented in issues?

@tahini tahini marked this pull request as draft February 20, 2024 22:29
@tahini
Copy link
Collaborator Author

tahini commented Feb 20, 2024

Retour en draft. J'ai un léger doute sur les performances, pour des grandes agences, je veux faire qq tests avant.

@tahini
Copy link
Collaborator Author

tahini commented Mar 11, 2024

Alors j'ai ajouté [enfin] une option de config pour désactiver les animations de carte. ça va rendre certains utilisateurs heureux et permettre aux gens comme moi avec des GPU moins performants de ne pas être trop demandant.

@tahini tahini marked this pull request as ready for review March 13, 2024 15:02
@tahini tahini force-pushed the deck-gl-pow branch 2 times, most recently from e7352a9 to a68c5fe Compare March 20, 2024 12:30
@tahini tahini mentioned this pull request Mar 22, 2024
@greenscientist
Copy link
Collaborator

@tahini So we don't forget, can you comment here on that is the current blocker for this PR

@tahini tahini marked this pull request as draft May 21, 2024 13:09
@tahini
Copy link
Collaborator Author

tahini commented May 21, 2024

Back to draft. It still needs the polygon selection tool to be complete. I worked on it friday, it works. I just need to fine-tune when to show it.

florencelauer and others added 23 commits May 24, 2024 15:21
Working layers using geojson data, events and custom shader

Co-authored-by: Wassim27 <mw-guellati@outlook.fr>
Co-authored-by: Gabriel Bruno <gabriel.bruno@polymtl.ca>
Co-authored-by: Florence Lauer <florence_lauer@outlook.com>
Co-authored-by: MohamedAli-M <mohamed-ali.messedi@polymtl.ca>
Co-authored-by: mahdiguermache <mahdi.guermache@polymtl.ca>
Co-authored-by: nik498 <54679682+nik498@users.noreply.github.com>
Current state: no background, simple geometries are displayed. All
enabled layers are displayed, with proper color if that color is in the
properties, otherwise, it is a default color.

There is no interaction with the layers and the circles are very small
when zoomed out. The selected shaders do not work.

The original layer descriptions in the layers.config.ts file have not
been changed or updated yet. The information they contain may be useful
for the Deck.gl data, but we'll need to take each field and see how to
define/type it for best Deck.gl support
For now, only three styles are available: OSM, positron and dark matter from CartoDB. Eventually, we can extend to also use MapTiler's tiles, but his requires an API key.
READMEs remain to be updated
The type is named `MapLayer` and another type `LayerConfiguration` is used
to configure the layer by the application. The configuration is meant to
be fixed by the application and does not change, except through
Preferences, if necessary.
Performance could be improved, but it works sufficiently well for now
TripsLayer is for animating a fading path, but we don't need that for general transit lines.
* Change the API of the event handlers
* Drop support of mapbox types in event handling
* Change the events to use the new API
* mouseDown/Move/Up are now onDrag and onDragEnd events, which happen
  only with a specific feature, these are easier
* There are simple tooltip events, which simplify popup management when
  only a simple text is required
* The `click` event is separate in `leftClick` and `rightClick`

TODO: The `path.hoverNode` and `path.unhoverNode` are not yet
functional, as they programatically need to create a popup or menu at a
specific location.

TODO2: Aesthetic changes are not taken into account, as zoom events (and
other layout events) are not yet properly handled

TODO3: Not all layers are refreshed, even though the collection they
display has elements that have changed. For example, saving a path does
not show the updated path. Deck.GL does a shallow comparison to
determine if the layer needs to be refreshed and probably the shallow
comparison does not show any change if the coordinates changed.
Deck gl does a shallow comparison of the data to determine if the layer
needs to be updated. So the data is not refreshed by default if one of
the features in the collection changed position.

We could add a `dataComparator` function, but this
will be executed for every re-render of the map. We instead use an
updateCount to save the number of times a layer has been updated
throught the `map.updateLayer[s]` event. This will cause the data to be
refreshed whenever the count is incremented.
Bring back all mapbox styles for circle-type layers.
Type the layer description for circle layers, with fields being either a
number/color, a function receiving the feature in parameter, or a
property getter to retrieve the value from a geojson property.
Bring back most of mapbox styles for line-type layers. The highlight
uses autoHighlight instead of changing the line width.

The 'animatedArrowPath' also share a good part of the configuration and
re-use the same logic for the PathLayer's data.
Bring back the mapbox styles for the polygon layers. With the lineColor
and fillColor, it is not necessary to have both a polygon and a stroke
layer, one layer does them all.
Deck.gl is now implemented in mainline Transition and the poc is not
necessary anymore.
Add the `featureMinZoom` property, which can be a function that takes
the feature in parameter and return the minimum zoom at which this
feature should be displayed.

Use this property for the transitPaths layer to recover the
functionality previously handled by the defaultFilter of mapbox.
Add the `canFilter` layer property. Because we need to know from the
start the number of filters to enable for a layer, as it is sent to the
shader and not updatable, even with the updateTrigger, this property
allows to set a placeholder filter for all features when no other filter
is set. See if this can be prevented somehow.

The PathMapLayerManager is renamed to TransitPathFilterManager because
it does not involve layers, but manages the filters. This class emits
the filter update events. The event is caught by the map, which updates
the layer accordingly.
What remains that still depended on mapbox are the popup manager, which
tracks which popup are currently opened and where, and the polygon draw
tool, which is a little known feature of transition and only used for
node selection.

Some code was commented, other removed, but those features are expected
to come back soon, tuned for deck.gl.
Defaults to `true`, but can be set in the user preferences. Layers can
then use the `map.enableMapAnimations` preferences value to disable
animations.
Add a prop to disable animation on the `AnimatedArrowPathLayer`. This
deactivates the animated frame request and arrow path time step, thus
saving the GPU from calculations. The previous speed divider of 0 also
had the effect of making the path not move, but the layer was still
refreshed for every frame, impacting GPU performances.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants