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

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

Closed
hdmm3 opened this issue May 16, 2024 · 9 comments
Closed

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

hdmm3 opened this issue May 16, 2024 · 9 comments

Comments

@hdmm3
Copy link

hdmm3 commented May 16, 2024

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
Copy link
Collaborator

dcooley commented May 16, 2024

Thanks for the PR - now merged

@dcooley dcooley closed this as completed May 16, 2024
@dcooley
Copy link
Collaborator

dcooley commented May 21, 2024

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

@dcooley dcooley reopened this May 21, 2024
@dcooley dcooley mentioned this issue May 21, 2024
dcooley pushed a commit that referenced this issue May 21, 2024
@dcooley
Copy link
Collaborator

dcooley commented May 21, 2024

something else was wrong so I've fully reverted this

@hdmm3
Copy link
Author

hdmm3 commented May 23, 2024

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:

  • At creation the viewState object will simply be set to the input initialViewState prop.
  • When using initialViewState, the onViewStateChange callback called during controller interaction, is internally followed by an update to the internal viewState here. This update will track changes to the viewState in a nested object viewState[viewId]. That way multiple views can save their own viewState independently. Most importantly, the current view state will be located in that nested object, instead of at the root viewState. Also the viewManager will be updated.
  • Currently, md_change_location overwrites the internal viewState through setProps({initialViewState}) effectively destroying the nested object created during onViewStateChange.
  • If a call to md_change_location is made after controller interaction, reading the values of the viewState object will pick stale values of the view instead of the most current ones.

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
Copy link
Collaborator

dcooley commented May 23, 2024

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
Copy link
Collaborator

dcooley commented Jul 1, 2024

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 added a commit that referenced this issue Jul 2, 2024
@dcooley
Copy link
Collaborator

dcooley commented Jul 2, 2024

@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
Copy link
Author

hdmm3 commented Jul 10, 2024

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
Copy link
Collaborator

dcooley commented Jul 16, 2024

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

@dcooley dcooley closed this as completed Jul 16, 2024
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

No branches or pull requests

2 participants