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

added canvas as option to the viewer #584

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

haoming29
Copy link

Public API Changes

The Viewer constructor now accepts canvas as an option in the parameter options.

Description

  • Added canvas as the parameter to the Viewer constructor so that the user can pass OffscreenCanvas to ROS 3D for performance improvement
  • Added related test to make sure Viewer correctly initialize with the canvas option

This is built upon PR #549 which will be closed when this PR opens.

#546

@haoming29
Copy link
Author

Hi @MatthijsBurgh, could you please review this PR at your convenience? Thanks.

src/visualization/Viewer.js Outdated Show resolved Hide resolved
src/visualization/Viewer.js Outdated Show resolved Hide resolved
src/visualization/Viewer.js Outdated Show resolved Hide resolved
src/visualization/Viewer.js Outdated Show resolved Hide resolved
src/visualization/Viewer.js Outdated Show resolved Hide resolved
tests/tests.js Outdated Show resolved Hide resolved
tests/tests.js Outdated Show resolved Hide resolved
tests/tests.js Outdated Show resolved Hide resolved
tests/tests.js Outdated Show resolved Hide resolved
@haoming29
Copy link
Author

@MatthijsBurgh Appreciate all the suggestions! Made the modifications and please help review it at your convenience. Thanks!

tests/tests.js Outdated Show resolved Hide resolved
@MatthijsBurgh
Copy link
Contributor

@Techming could you please edit your first commit to exclude the changes in the build folder?

@haoming29
Copy link
Author

@MatthijsBurgh Sorry about that. Reverted build files to the previous commit. (Apologies for the typo in the commit message)

@MatthijsBurgh
Copy link
Contributor

@Techming there are 2 test folders. One is just for running manually in your browser. The other one is running in CI too. You didn't add the test there. I did add it there. Now CI fails.

@haoming29
Copy link
Author

haoming29 commented Nov 22, 2022

I’m working on identifying the issue and wrote additional tests to make sure the Viewer itself can pass the CI test. Could you help approve the workflow? Thanks. @MatthijsBurgh

@haoming29
Copy link
Author

From the CI error log it seems that the Viewer class fails the CI without canvas passed in as the param. I will confirm locally if the changes of in my code have effects on the result. @MatthijsBurgh

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.

2 participants