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

Viewer.destroy not checking if ScreenSpaceEventHandler is defined #10576

Closed
TJPovey opened this issue Jul 20, 2022 · 3 comments · Fixed by #11840
Closed

Viewer.destroy not checking if ScreenSpaceEventHandler is defined #10576

TJPovey opened this issue Jul 20, 2022 · 3 comments · Fixed by #11840
Labels
cleanup good first issue An opportunity for first time contributors

Comments

@TJPovey
Copy link

TJPovey commented Jul 20, 2022

Sandcastle example:
https://sandcastle.cesium.com/#c=hVA9a8MwEP0rhztYhiDTuY4ppIUOgQ6BTl4U6ZqKyJIrnRzc0v9eOUogoYFqu/dx7+mks4Fg1HhAD0uweIAVBh17/nbEWFfI47xyloS26LuieuhsZ7OHB+kR7WYQEp9HtPQirDIJVxjIu4llsXIy9onlnxH9tEGDkpxn5d1JlsPKiguljmvWOhCmNFZKo+W+XACrYNnCd2cBZCrtDHLjduyfGjo85QRUrJq7wOmzVwV/cstiUTSBJoPtrJvfo+4H5wmiN4zzmrAfjCAM9TbKPRKXIeSlAE19aW2UHkGr5Y3zgTQihMS8R2M2+gu7om3qpP9jNU4obXevI3ojpln2cd+uM8g5b+o03naSc2YrUtiZ2UYiZzN5dfMkOV0IMtDUWXt2Xuz/BQ

If the viewers ScreenSpaceEventHandler has been destroyed prior to the viewer being destroyed, unminifed cesiumjs is throwing a developer error:
The object is destroyed...
The viewers destroy method is not checking if the handler has been destroyed prior to using it, like it is for other fields:

this.screenSpaceEventHandler.removeInputAction(

Happy to create a PR or remove the issue if it is desired behaviour. Although it does look like the viewer is only removing two input actions and not actually destroying the handler:

  this.screenSpaceEventHandler.removeInputAction(
    ScreenSpaceEventType.LEFT_CLICK
  );
  this.screenSpaceEventHandler.removeInputAction(
    ScreenSpaceEventType.LEFT_DOUBLE_CLICK
  );
@ggetz
Copy link
Contributor

ggetz commented Jul 21, 2022

@TJPovey Thanks for the report! Though I think there was an underlying assumption that the screenSpaceEventHandler wouldn't be destroyed separately, I don't see why a simple verification in the destroy method would hurt.

@ggetz ggetz added cleanup good first issue An opportunity for first time contributors labels Aug 3, 2022
@nayangoyal
Copy link

nayangoyal commented Sep 7, 2023

Hi, @ggetz can you assign this issue to me? I want to work on this issue.

@ggetz
Copy link
Contributor

ggetz commented Sep 7, 2023

Hi @nayangoyal, great! Please go ahead and get started by taking a look at CONTRIBUTING.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup good first issue An opportunity for first time contributors
Projects
None yet
3 participants