Add network spawning for dynamic gltf-model objects. Make equippables work for network spawned objects. #4071

HexaField · 2021-11-13T02:04:36Z

Summary

A summary of changes being made in this PR

Checklist

Pre-push checks pass npm run check
- Linter passing via npm run lint
- Unit & Integration tests passing via npm run test:packages
- Docker build process passing via npm run build-client
Changes have been manually QA'd
Changes reviewed by at least 2 approved reviewers

References

References to pertaining issue(s)

Reviewers

@HexaField @speigg @NateTheGreatt

…object flag is added on editor side. Update incomingNetworkReceptor to handle these dynamic objects.

…ked instantiated objects.

…ish code.

NateTheGreatt

looks good, @hamzzam ! just a few questions/comments. the only changes i'd like to see before merging is to have those hasComponent calls moved into removeComponent. we might also want some of those console.log calls removed, too.

NateTheGreatt · 2021-11-24T20:20:50Z

packages/engine/src/interaction/functions/equippableFunctions.ts

-    !hasComponent(equippedEntity, EquippedComponent)
-  ) {
-    addComponent(equipperEntity, EquipperComponent, { equippedEntity, data: {} })
+  if (!hasComponent(equipperEntity, EquipperComponent) && !hasComponent(equippedEntity, EquippedComponent)) {


for my own understanding, is this if statement here to prevent the existing component data from being overwritten?

NateTheGreatt · 2021-11-24T20:37:14Z

packages/engine/src/interaction/systems/EquippableSystem.ts

-    // } else {
+  const world = useWorld()
+
+  matches(action).when(NetworkWorldAction.setEquippedObject.matchesFromAny, (a) => {


what's the difference between .matches and .matchesFromAny? i always forget ><

.matches will match an action only when it comes from a trusted source (e.g., the host, or itself)
.matchesFromAny will match an action no matter who sent it

In general .matchesFromAny is unsafe, and .matches should be preferred

@NateTheGreatt happy to rename these for clarity if you can think of better names

packages/engine/src/interaction/systems/EquippableSystem.ts

packages/engine/src/networking/functions/incomingNetworkReceptor.ts

NateTheGreatt · 2021-11-24T20:47:36Z

packages/engine/src/interaction/systems/InteractiveSystem.ts

+      if (hasComponent(entity, BoundingBoxComponent)) {
+        // TODO: Does the Box3 object need to be destroyed before this?
+        removeComponent(entity, BoundingBoxComponent)
+      }
+      if (hasComponent(entity, InteractiveFocusedComponent)) {
+        removeComponent(entity, InteractiveFocusedComponent)
+      }
+      if (hasComponent(entity, SubFocusedComponent)) {
+        removeComponent(entity, SubFocusedComponent)
+      }


are these hasComponent checks necessary if it's removing the component anyways? if it's needed, perhaps we should add the hadComponent check to the removeComponent function instead

@NateTheGreatt Does bitecs allow removeComponent if the entity does not have the component?

Yeah I was of the assumption that bitECS does not allow removeComponent if the component does not exist. But I just ran a test it does not throw any errors. So I will update the code and remove the extra hasComponent checks.

Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

…or.ts Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

hamzzam · 2021-11-25T20:03:27Z

looks good, @hamzzam ! just a few questions/comments. the only changes i'd like to see before merging is to have those hasComponent calls moved into removeComponent. we might also want some of those console.log calls removed, too.

Removed the console.logs and hasComponent checks. Let me know if anything else needs to be updated. Thanks

HexaField · 2021-11-26T04:24:34Z

Are you able to provide a guide to show the functionality for this? I tried using the collision cube to create an equippable and there is an issue with the new interactable UI and i cant seem to equip anything.

hamzzam · 2021-11-26T07:55:45Z

Are you able to provide a guide to show the functionality for this? I tried using the collision cube to create an equippable and there is an issue with the new interactable UI and i cant seem to equip anything.

It was working fine before the latest dev merge. Let me try to debug

NateTheGreatt

QA steps would be great. i forgot to add that to the PR template

hamzzam · 2021-11-26T12:48:13Z

@HexaField @NateTheGreatt I tested it out, the equippables functionality is working fine. There are some errors in the interactables logic due to which the text and highlighting are not working. I tried adding some sanity checks but that didn't help. Probably the dev who worked on it will need to fix those.

To test the interactables you should do the following:

In the editor, add a model node and set its interaction type to Equippable. Save.
Then in the scene, take the avatar near the equippable model and press E. The object will attach to the avatar hand.

NateTheGreatt · 2021-11-26T21:45:17Z

thanks for the repro steps @hamzzam! I tested it out and there seems to be an issue with the item following the hand bone after pickup.

hamzzam · 2021-11-27T11:49:32Z

thanks for the repro steps @hamzzam! I tested it out and there seems to be an issue with the item following the hand bone after pickup.

Hey @NateTheGreatt that's because the getHandTransform function is returning a fixed hand position currently. There is a TODO present to use the actual hand position when the animation rig is in. And I think that is in the system now, should I try fixing this? will need to look into the animation rig system. @HexaField

HexaField · 2021-11-27T22:33:17Z

Hey @NateTheGreatt that's because the getHandTransform function is returning a fixed hand position currently. There is a TODO present to use the actual hand position when the animation rig is in. And I think that is in the system now, should I try fixing this? will need to look into the animation rig system. @HexaField

Yes I think we are now in a place where this should work.

hamzzam · 2021-11-29T07:02:35Z

Hey @NateTheGreatt that's because the getHandTransform function is returning a fixed hand position currently. There is a TODO present to use the actual hand position when the animation rig is in. And I think that is in the system now, should I try fixing this? will need to look into the animation rig system. @HexaField

Yes I think we are now in a place where this should work.

Ok. I will make a separate PR for that, we can get this PR approved if that is the only thing left.
Or let me know if you think this change must be included in this PR.

NateTheGreatt

Ok. I will make a separate PR for that, we can get this PR approved if that is the only thing left.
Or let me know if you think this change must be included in this PR.

sounds good to me 👍

try to get equip object working again

6776035

HexaField changed the title ~~try to get equip object working again~~ Fix Grabbables Nov 13, 2021

Pick and move equiabble working again for local player.

937ae3e

hamzzam added 6 commits November 16, 2021 23:57

Mark all model node objects as network objects for now until dynamic …

3aad2b9

…object flag is added on editor side. Update incomingNetworkReceptor to handle these dynamic objects.

Merge branch 'dev' into fix-grabbables

7d431ff

Add support for making dynamic/networked gltf-model objects as networ…

6b68d84

…ked instantiated objects.

Update setEquippedObject network action to be triggerable from clients.

932e9a9

Euipped object state now working across network.

0e669dc

Fix attachment point logic for equippables.

7ca2a23

hamzzam changed the title ~~Fix Grabbables~~ Add network spawning for dynamic gltf-model objects. Nov 23, 2021

hamzzam changed the title ~~Add network spawning for dynamic gltf-model objects.~~ Add network spawning for dynamic gltf-model objects. Make equippables work for network spawned objects. Nov 23, 2021

hamzzam added 6 commits November 23, 2021 21:34

Refactor network spawning logic of dynamic objects to remove dev/hack…

5267d6a

…ish code.

Add missing param in equip entity network action.

82534b9

Merge branch 'dev' into fix-grabbables

876100b

hotfix

998d99c

Remove extra logs.

9b67044

Merge branch 'dev' into fix-grabbables

1a7abcf

hamzzam marked this pull request as ready for review November 24, 2021 11:07

Merge branch 'dev' into fix-grabbables

23f855f

NateTheGreatt suggested changes Nov 24, 2021

View reviewed changes

hamzzam and others added 7 commits November 26, 2021 00:14

Update packages/engine/src/interaction/systems/EquippableSystem.ts

670d041

Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

Update packages/engine/src/interaction/systems/EquippableSystem.ts

dc28f4e

Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

Update packages/engine/src/interaction/systems/EquippableSystem.ts

d03f898

Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

Update packages/engine/src/networking/functions/incomingNetworkRecept…

7fffe7b

…or.ts Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

Update packages/engine/src/networking/functions/incomingNetworkRecept…

166abeb

…or.ts Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

Update packages/engine/src/networking/functions/incomingNetworkRecept…

0b8da87

…or.ts Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

Update packages/engine/src/networking/functions/incomingNetworkRecept…

401fd6b

…or.ts Co-authored-by: Nate Martin <mrtn.nathaniel@gmail.com>

hamzzam added 2 commits November 26, 2021 00:44

Remove extra console logs.

d19d0d2

Remove hasComponent checks before calling removeComponent.

1ac7f0f

HexaField and others added 2 commits November 26, 2021 09:07

Merge branch 'dev' into fix-grabbables

58f51fd

Merge branch 'dev' into fix-grabbables

c0f8b7d

NateTheGreatt approved these changes Nov 26, 2021

View reviewed changes

NateTheGreatt suggested changes Nov 26, 2021

View reviewed changes

Merge branch 'dev' into fix-grabbables

6c16e47

HexaField and others added 2 commits November 29, 2021 19:19

Merge branch 'dev' into fix-grabbables

5a08808

Merge branch 'dev' into fix-grabbables

48c5d4f

NateTheGreatt approved these changes Nov 29, 2021

View reviewed changes

speigg merged commit 8c8f90b into dev Nov 29, 2021

speigg deleted the fix-grabbables branch November 29, 2021 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network spawning for dynamic gltf-model objects. Make equippables work for network spawned objects. #4071

Add network spawning for dynamic gltf-model objects. Make equippables work for network spawned objects. #4071

HexaField commented Nov 13, 2021

NateTheGreatt left a comment •

edited

Loading

NateTheGreatt Nov 24, 2021

hamzzam Nov 25, 2021

NateTheGreatt Nov 24, 2021

speigg Nov 25, 2021 •

edited

Loading

speigg Nov 25, 2021

NateTheGreatt Nov 24, 2021

speigg Nov 25, 2021

hamzzam Nov 25, 2021

hamzzam Nov 25, 2021

hamzzam commented Nov 25, 2021

HexaField commented Nov 26, 2021

hamzzam commented Nov 26, 2021

NateTheGreatt left a comment

hamzzam commented Nov 26, 2021

NateTheGreatt commented Nov 26, 2021 •

edited

Loading

hamzzam commented Nov 27, 2021

HexaField commented Nov 27, 2021

hamzzam commented Nov 29, 2021

NateTheGreatt left a comment

Add network spawning for dynamic gltf-model objects. Make equippables work for network spawned objects. #4071

Add network spawning for dynamic gltf-model objects. Make equippables work for network spawned objects. #4071

Conversation

HexaField commented Nov 13, 2021

Summary

Checklist

References

Reviewers

NateTheGreatt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speigg Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamzzam commented Nov 25, 2021

HexaField commented Nov 26, 2021

hamzzam commented Nov 26, 2021

NateTheGreatt left a comment

Choose a reason for hiding this comment

hamzzam commented Nov 26, 2021

NateTheGreatt commented Nov 26, 2021 • edited Loading

hamzzam commented Nov 27, 2021

HexaField commented Nov 27, 2021

hamzzam commented Nov 29, 2021

NateTheGreatt left a comment

Choose a reason for hiding this comment

NateTheGreatt left a comment •

edited

Loading

speigg Nov 25, 2021 •

edited

Loading

NateTheGreatt commented Nov 26, 2021 •

edited

Loading