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 spawner issues #6158

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Fix spawner issues #6158

merged 1 commit into from
Aug 9, 2023

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Jul 10, 2023

This PR addresses some issues with the spawner and how the spawner component and the spawned objects are created (static, recentered, resized) and reorders the physics related systems so that we give a chance to the physics system to create bodies and shapes before they are processed by other systems.

@takahirox
Copy link
Contributor

Thanks for working on it. Would you mind if writing the details in the PR comment for reviewers and future readers to follow the change and background?

@pattersonbl2 pattersonbl2 temporarily deployed to smoke July 24, 2023 15:39 — with GitHub Actions Inactive
@takahirox
Copy link
Contributor

takahirox commented Jul 29, 2023

I confirmed that this change resolves the spanwer object problem that spawner object isn't renderred (at the correct position) for now.

reorders the physics related systems so that we give a chance to the physics system to create bodies and shapes before they are processed by other systems.

Sounds like it's a good fix.

I add some comments as just question for my study. You can ignore them if you are busy.

Honestly I'm not familiar with physics and spawners so can't review in details. But spawner objects will be rendered correctly with this change so I think good to go with the change.

If more detailed review is needed, please ask other devs (maybe @johnshaughnessy).

spawnerObj.visible = true;

// TODO we should come up with a nicer way to get at the media that was loaded by a MediaLoader
yield* animateScale(world, spawnerObj.children[0]!.eid!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for my study: Here can be removed because of animateLoad: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as we are already animating the spawned object I don't see the point on animating explicitly.


addComponent(world, ObjectSpawner, eid);
ObjectSpawner.src[eid] = APP.getSid(props.src);
ObjectSpawner.flags[eid] = props.mediaOptions?.applyGravity ? OBJECT_SPAWNER_FLAGS.APPLY_GRAVITY : 0;

inflateRigidBody(world, eid, {
mass: 0,
type: Type.STATIC,
Copy link
Contributor

@takahirox takahirox Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for my study: Spawner object should be static (no local matrix update except for initialization) so rigid body type should be static. Is my understanding correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my understanding. Spawners are static as their main action is to spawn an object and are not draggable. Also that's how we were setting them up in AFrame:
https://github.com/mozilla/hubs/blob/5f6e7a62ab998a60895a30dc686ae6db94fe9c5b/src/gltf-component-mappings.js#L323

@keianhzo keianhzo merged commit 1907267 into master Aug 9, 2023
10 of 12 checks passed
@keianhzo keianhzo deleted the bitecs-spawner-fix branch August 9, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants