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

Avoid picking in overlays before deck is initialized #7668

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JannikGM
Copy link
Contributor

@JannikGM JannikGM commented Feb 20, 2023

Background

I've originally reported a bug on Slack: https://deckgl.slack.com/archives/C34AC5MSQ/p1676629109034669
The picking functions were used by the mapbox event handlers before deck was fully initialized (which will be fixed by #7723).

Also, while looking into it, I noticed that the explicit picking support provided by the overlays also isn't guarded against incorrect use before deck is initialized.
Because the user of the overlay doesn't have direct deck access either, I proposed to forward isInitialized.

I've also added an assert in case this._deck doesn't exist yet to the picking functions and made comments more consistent.

Change List

  • Add isInitialized to Mapbox- and GoogleMapsOverlay
  • Add types to GoogleMapsOverlay picker
  • Consistent picker comments in Mapbox- and GoogleMapsOverlay

None of this was tested
I'm not sure how to run automated tests for the deck.gl codebase. yarn bootstrap fails with gyp not able to find python (which should be python3 on this macOS machine). yarn test failed during linter step in untouched code.
All my deck.gl projects use another private library which has hardcoded dependency on a specific deck.gl version, so manual testing would be cumbersome.

@@ -43,6 +43,11 @@ export default class GoogleMapsOverlay {

/* Public API */

/** Equivalent of `deck.isInitialized`. */
get isInitialized(): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice if the user wouldn't have to worry about this, could we not instead have pickObject and friends return null when deck is uninitialized?

Copy link
Contributor Author

@JannikGM JannikGM Feb 21, 2023

Choose a reason for hiding this comment

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

I'm not too happy about this design myself.

However, this is also how it works for the Deck class: https://deck.gl/docs/api-reference/core/deck#isinitialized (where I also don't like isInitialized, it caught me off-guard).

Personally I think forwarding picking functions (or deck features in general) to the overlays shouldn't even be necessary.
Raw access to the deck could be provided instead, so it's the users responsibility to use isInitialized on the deck.

But it is how it is (forwarded calls, and isInitialized on the deck to avoid errors in picking), so I proposed to add this to the overlays for consistency.


To summarize the design alternatives:

  • One could potentially get rid of isInitialized on Deck (which might have other challenges) and keep overlay code mostly unchanged.
    • Return null on the picking before initialization.
      This might lead to issues if the user only picks once per camera move or similar. They wouldn't know if nothing was picked, or if the picking result is unavailable.
    • Return undefined on the picking before initialization.
      This would enable to see why picking failed (null: no object / undefined: not initialized or some other error).
      However, I don't think undefined is used in deck APIs normally.
  • Avoid forwarding calls and provide something like function getDeck() or get deck().
    • Return null from getter until isInitialized which requires a lot of code in callee.
    • Return deck immediately and responsibility stays with callee to check deck.isInitialized.

Most of these are breaking changes to the public API (typing and/or behaviour).


I think I'll move the bugfixes in handleMouseEvent to a separate bugfix PR (..as it affects real world applications and there's no workaround for users).
This PR would only have the more controversial changes to the public APIs of the overlays (..which aren't as critical).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to note that the next planned release is v9.0, so we can make breaking changes if it makes more sense.

@coveralls
Copy link

Coverage Status

Coverage: 90.042% (-0.01%) from 90.055% when pulling e86b529 on JannikGM:overlay-picking into 90dfbe1 on visgl:master.

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

4 participants