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: TresGroup components emit children mesh events #439

Closed
wants to merge 7 commits into from

Conversation

garrlker
Copy link
Collaborator

Not quite done yet. It works, but only for children components that also have event handlers wired onto them.
It's seems to be ignoring the rest of the children.

Also random but it seems like mouse events are firing off sporadically?

I'm including TheExperience bug recreation from the issue for now, but will remove it before merge

Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for tresjs-docs ready!

Name Link
🔨 Latest commit e78c162
🔍 Latest deploy log https://app.netlify.com/sites/tresjs-docs/deploys/6579fee499b7440008809a83
😎 Deploy Preview https://deploy-preview-439--tresjs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@garrlker garrlker changed the title TresGroup components emit children mesh events fix: TresGroup components emit children mesh events Nov 10, 2023
garrlker and others added 2 commits November 10, 2023 23:01
… we check if an object isGroup. If it is, we run through it's children and set their event listeners as the parents. If the child already has event listeners wired up, then we combine the functions
@alvarosabu
Copy link
Member

Hey buddy @garrlker any update on this one?

@garrlker
Copy link
Collaborator Author

@alvarosabu just saw your comment. I sent you a DM about this but for anyone else seeing this post

I've been working on a different approach and I'm hoping to push those changes later this week. It's looking to be much simpler, require less type changes, "just work" TM, and also match "convention" to Vue's native event bubbling.

The current solution that is pushed up was more convoluted to support than I expected plus @Tinoooo found some issues with it that could be difficult to work around.

@alvarosabu alvarosabu added the bug Something isn't working label Dec 13, 2023
@garrlker garrlker marked this pull request as ready for review December 29, 2023 23:20
@alvarosabu alvarosabu mentioned this pull request Jan 2, 2024
8 tasks
@Tinoooo
Copy link
Contributor

Tinoooo commented Jan 2, 2024

@garrlker I finally found some time to check out your PR. Sorry for the huge delay.

I have three topics I want to discuss.

1️⃣ Parent elements (the group) only receive events if the child element has event listeners attached to them. I know that Threlte and R3F work this way, but I think this behavior is not quite intuitive to users. But this might be just me. On the other side I don't have a better solution in mind, so maybe the behavior is the way to go.

2️⃣ Using pointer-leave on a TresGroup also triggers, when the mouse leaves a child object but not the group. Users might find this behavior unintuitive.

3️⃣ This should work with primitives (like shown here) too.

Both topics require us to first define how the event handling/bubbling should work. What are you opinions about this @garrlker @alvarosabu @JaimeTorrealba? If this discussion requires extensive time, we could schedule a call to address it.

small update:
The event bubbling has an advantage. You can decide in the callback wether or not to stop the propagation or not. This does not work as easy with our blocks-pointer-events prop.

@alvarosabu
Copy link
Member

Hi, @Tinoooo thanks for jumping in, and thanks both @garrlker for taking care of this one.

I have done benchmarking and created a simple app on the 3 major renderers to check the differences between them and understand the problem (without taking this PR into consideration):

R3F and Threlte share the most similarities, especially on the event topic vs Tres. This is not necessarily a bad thing but I think we should agree and maybe ask the community what behavior they are expecting to decide if its a bug.

With that being said I think its an area with a lot of room for improvement. I'm up for a pair programming session together if you like! We can do it on discord on the after-work channel or even a bug-hunting stream on Twitch, as you prefer.

Let me know what do you think

@alvarosabu
Copy link
Member

@garrlker quick small feedback:

  1. when creating a branch please create it under feature or bugfix depending on the nature of the issue 😊, this branch should be bugfix/426-groups-emit...
  2. I normally recommend to create a route on the playground to test out instead of using the TheExperience, specially useful for code reviews and avoid conflicts.

@garrlker
Copy link
Collaborator Author

garrlker commented Jan 3, 2024

@Tinoooo thank you for diving into this so thoroughly. I missed those cases

100% agree on us clearly defining how this should work and asking the community.
IMO while we don't have to follow R3F/Threlte if they are both aligning on similar patterns then we should follow common convention.

We can still implement it in a more Vue-ish way, Ex. we should have .prevent and .stop event modifiers to handle Threejs boilerplate. There are possibly other common patterns we could shorten, but those 2 are easy layups

@alvarosabu Gotcha, ty for the tips

I'm also down for a call and/or pair programming to get this finished

@garrlker garrlker closed this Jan 23, 2024
@garrlker
Copy link
Collaborator Author

Closing for #515

Will continue work there

@alvarosabu alvarosabu mentioned this pull request Feb 7, 2024
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants