SymbolixAU / mapdeck

R interface to Deck.gl and Mapbox
https://symbolixau.github.io/mapdeck/articles/mapdeck.html
362 stars 40 forks source link

mapdeck_view() doesn't use current viewState properties #382

Closed hdmm3 closed 4 months ago

hdmm3 commented 6 months ago

I've been trying to use mapdeck_view() to individually update some map parameters, like the bearing or zoom level. However, after manually zooming, dragging, tilting, if I call something like mapdeck_view(pitch = 45) the rest of the viewState parameters will be reset to the latest values programmatically defined for the viewState, instead of the current values.

After a quick inspection, I noticed that the underlying function here is getting the current lon, lat, pitch, bearing, zoom, etc, directly from the viewState whose values don't seem to change when interacting with the map. The current values seem to be nested inside viewState.map instead. So for instance retrieving the current pitch could be done through:

currentPitch = pitch === null ? window[ map_id + 'map'].viewState.map.pitch : pitch;

I'll submit a PR for this issue.

dcooley commented 6 months ago

Thanks for the PR - now merged

dcooley commented 6 months ago

Something in this commit causes the map to freeze, so I've reverted to the commit before this.

dcooley commented 6 months ago

something else was wrong so I've fully reverted this

hdmm3 commented 6 months ago

Hi @dcooley, first of all apologies for those half-baked PRs. This time I'll share my thoughts first. I've been digging about the intended behaviour of deck.gl when using either initialViewState or viewState props. Here are some of my interpretations of what's going on when using the initialViewState prop:

In order to avoid fully overwriting the viewState, the initialViewState prop could be set in md_change_location as follows:

window[map_id + 'map'].setProps({
      initialViewState: {
        ...window[map_id + 'map'].viewState,
        [map_id]: {
          longitude: currentLon,
          latitude: currentLat,
          zoom: currentZoom,
          maxZoom: currentMaxZoom,
          minZoom: currentMinZoom,
          maxPitch: currentMaxPitch,
          minPitch: currentMinPitch,
          pitch: currentPitch,
          bearing: currentBearing,
          transitionInterpolator: transition === "fly" ? new deck.FlyToInterpolator() : new deck.LinearInterpolator(),
          transitionDuration: duration,
          controller: true}        
      }
    });

As such, the current viewState for the map_id could be retrieved using viewManager.getViewState(), as follows:

currentViewState = window[map_id + 'map'].viewManager?.getViewState(map_id) || window[map_id + 'map'].viewState;

getViewState() seems to be the safest approach. I'm not sure if after deck.gl 8+ the "default-view" viewId will even occur when the DeckGL views prop defines views with id as it's done with mapdeck.

I hope this helps!

dcooley commented 6 months ago

Thanks for the extra detail. And I've been meaining to revisit the viewState logic / code so thanks for bringing this up. When I first implemented it I had a lot of issues getting it to work in both a shiny & non-shiny environment, and getting the map & layers to update correctly.

I'll try and set some time aside next week to look at this in more detail.

dcooley commented 4 months ago

reprex for this:

library(shiny)
library(sinydashboard)

set_token(secret::get_secret("MAPBOX"))

ui <- shinydashboard::dashboardPage(
    header = shinydashboard::dashboardHeader()
    , sidebar = shinydashboard::dashboardSidebar(
        shiny::actionButton(inputId = "viewState", label = "Pitch")
    )
    , body = shinydashboard::dashboardBody(
        mapdeck::mapdeckOutput(
            outputId = "map"
        )
    )
)

server <- function(input, output, session) {

    observeEvent({input$viewState},{
        mapdeck::mapdeck_update(map_id = "map") %>%
            mapdeck::mapdeck_view(pitch = 45)
    })

    output$map <- mapdeck::renderMapdeck({
        mapdeck::mapdeck() %>%
            mapdeck::add_scatterplot(
                data = capitals
            )
    })
}

shiny::shinyApp(ui, server)
dcooley commented 4 months ago

@hdmm3 I've finally managed to look at this again and added a potential solution to the branch issue382. If you get a chance can you test it and see if it works for you?

Notes:

hdmm3 commented 4 months ago

Hi @dcooley, I just tested your commit in my project and it seems to be working perfectly. Let me know if you would like me to run any additional tests.

dcooley commented 4 months ago

great - I think it's now working as intended so I'll merge into master