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

fix(WidgetManager): handle touch support #2506

Merged
merged 2 commits into from
Sep 20, 2022
Merged

Conversation

floryst
Copy link
Collaborator

@floryst floryst commented Jul 7, 2022

Context

FYI @finetjul. I'm still investigating, but here is something that might work.

Results

  • touch should work on widgets
  • multiple widget managers should not freeze

Changes

  • Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • Tested widgets:
    • polyline widget with an orientation marker widget

@finetjul
Copy link
Member

finetjul commented Jul 8, 2022

LGTM

@floryst
Copy link
Collaborator Author

floryst commented Jul 8, 2022

There is still a performance issue when multiple widget managers exist in a render window.

@floryst
Copy link
Collaborator Author

floryst commented Jul 8, 2022

Some of the performance issues stem from #2484. I have a fix for that, which should benefit this PR.

Update: that performance gain was only for a very specific test. Still getting some slowness with multiple widget managers.

@floryst floryst marked this pull request as ready for review September 13, 2022 21:10
@floryst floryst force-pushed the widgetmanager-touch branch 2 times, most recently from 740a959 to 520c12d Compare September 13, 2022 21:16
@floryst
Copy link
Collaborator Author

floryst commented Sep 13, 2022

I think this is good to go. I've tested this on a Chromium-based browser on a smartphone using a polyline wiget example that has been modified to include the orientation marker widget.

@floryst
Copy link
Collaborator Author

floryst commented Sep 14, 2022

I've updated the PR with the following changes:

  • event handlers now return macro.VOID. handleEvent doesn't since it's async, so I opted to wrap the call to handleEvent in a separate function.
  • model._currentUpdateSelectionCallID is used to only allow the most recent updateSelection call to be processed.

@floryst
Copy link
Collaborator Author

floryst commented Sep 19, 2022

@finetjul can I get one more review? I'm set with this implementation, since it should work without getting into the weeds on asyncifying the event handling.

@finetjul
Copy link
Member

LGTM

convert() is a hot function, so avoid allocating/GC of the rgb array.
This adds functionality to only process the latest selection request.
@floryst floryst merged commit f9b9047 into master Sep 20, 2022
@floryst floryst deleted the widgetmanager-touch branch September 20, 2022 17:54
@github-actions
Copy link

🎉 This PR is included in version 25.8.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Automated label label Sep 20, 2022
@juan-crypto-bot
Copy link

Hello, I have a stl file that renders and when I add a widget the movement is frozen, I can't move the stl file and zoom it.
I try widgetManager.disablePicking(), widgetManager.delete() and it still does not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Automated label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants