AdaRoseCannon / aframe-xr-boilerplate

Get started quickly with VR and AR using AFrame
https://aframe-xr-starterkit.glitch.me/
MIT License
110 stars 20 forks source link

Update navmesh object whenever on child attached/detached #27

Closed Elettrotecnica closed 6 months ago

Elettrotecnica commented 6 months ago

Dear maintainers,

I have implemented reacting to child attached/detached on simple-navmesh-constraint for my own use case. I understand this was somewhat on the todo list, so I leave it here :-)

All the best

Elettrotecnica commented 6 months ago

Dear @vincentfretin, I have implemented your suggestions. I have also a small refactoring in my stash that should improve the array allocation situation, I could push it here directly, rather than in a new PR, as you wish :-)

vincentfretin commented 6 months ago

You should put this.needsUpdate = false; in init(), not true, otherwise it will execute update twice initially. Let's first merge this PR, then you can create other PRs after that one. Thanks!

vincentfretin commented 6 months ago

Or rename update to something else, and create an update method that just set this.needsUpdate = true and don't initialize it in init.

Elettrotecnica commented 6 months ago

I went with not initializing at all, should work as intended

vincentfretin commented 6 months ago

Oh sorry, it probably wasn't an issue because you set this.needsUpdate = false; in the end of update(). Ok good let's merge this.

Elettrotecnica commented 6 months ago

I threw in an extra little improvement: we skip the match altogether if we are already supposed to update.

vincentfretin commented 6 months ago

I see, even better!

vincentfretin commented 6 months ago

Actually there is something that bother me, calling update() on needsUpdate, will execute again this.lastPosition = null; and the this.xzOrigin = ... lines that are not needed. so probably better to have the this.objects and this.excludes in a separate method. What do you think?

Elettrotecnica commented 6 months ago

I have to admit that I wouldn't know the full consequences of recomputing those two values, but let's stay on the safe side: I have split the objects update from the actual component's update.

vincentfretin commented 6 months ago

Thank you.