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

Allow networked entities to become children of a non-networked parent #401

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Agupta00
Copy link

@Agupta00 Agupta00 commented Mar 12, 2023

What

Here we add the ability for networked entities to become children of non-networked entities (Ie, allow them to end up somewhere other than the root a-scene)

#400

Why

For example, imagine each client has a local platform that is not networked, but we want entities to become children off. Right now there is no way from my understanding to do this within the general API.

Before

This is what normally would happen in the current NAF API

before.mp4

After

The other client ends up on the platform (because it is a child of it)

Screen.Recording.2023-03-30.at.11.21.07.AM.mp4

@@ -427,6 +428,7 @@ AFRAME.registerComponent('networked', {
syncData.template = data.template;
syncData.persistent = data.persistent;
syncData.parent = this.getParentId();
syncData.attachToParentId = data.attachToParentId;
Copy link
Member

Choose a reason for hiding this comment

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

I see we already have syncData.parent here, so adding a new field to sync a parent id seems redundant. I think it would be better to modify getParentId and initNetworkParent here, and keep the addEntityToPage(entity, parentId) signature.

@@ -108,7 +111,8 @@
including all applicable child elements. However, because we don't need to see our own avatar, we use the
`attachTemplateToLocal:false` option. This makes our local copies invisible on our machine, but visible on everyone else's.
-->
<a-entity id="rig" movement-controls="fly:true;" spawn-in-circle="radius:3" networked="template:#rig-template;">
<a-entity id="rig" movement-controls="fly:true;" spawn-in-circle="radius:3" networked="template:#rig-template; attachToParentId:#platform;">
Copy link
Member

Choose a reason for hiding this comment

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

Instead of modifying the basic example. Can you create a new example where you absolutely need your changes.
For this modified example, instead of attachToParentId:#platform;" you can really just use position="0 10 0" on rig and it will be absolutely the same.

Copy link
Author

@Agupta00 Agupta00 Mar 14, 2023

Choose a reason for hiding this comment

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

Yeah sure, did you have an idea of what example would be good? Perhaps I can change it so the client can manually set the hight of the platform local to his own computer. Then I think you would absolutely need this change. Does that work?

Copy link
Member

Choose a reason for hiding this comment

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

I'm asking you because I don't know which use case those changes would really be useful. If I merge changes, I want an example where the changes are necessary and the example can't be achieved in another way.
Having the height of the platform configurable doesn't change anything, you change both position y on rig and platform and that would do it without the changes.
Didn't you have a real use case for those changes?
Your modified example seems the wrong use case to me. What about a second platform, where the avatars can go, you will need to change the parent of your avatar, how would it work? Generally you would not put your avatar rig as a parent of another entity but use a root entity, you would then update the avatar rig according to where the avatar is. This is how it works with the two different navmesh components we have.

Copy link
Author

Choose a reason for hiding this comment

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

Apologies in advance if I misunderstood anything.., here is why I think my example works.

Example: Player A's platform's y=10, Player B's platform is at y=20.

On Player B's local client, how would you ensure that Player A ends up on my platform? If you set Player A rig entity position to y=20, then the next time Player A moves, the rig's position would get updated back to the original y height since its rig is networked entity right? If it is still unclear I can make the example and perhaps you can try it.

(side note): I tried setting the position of a networked entity (from another client) and it did not update, maybe there is logic that prevents this anyways?

Real Life Example

Here is a video of a real use case, I can attach the scene code if it helps, but in summary, each client has a sort of "platform" whose position is local the client. In the video the "platform" is the green outline, which is tracked and changed locally on the client. A different phone viewing the same target can have a different position for the "platform".

The thing I care about syncing is the robot's positions, but only relative to the "platform". You can think of the "platform" as the root of my networked scene.

example.mp4
  <a-scene>
    <a-entity id="platform"> //local client can set this to anything. The position here only makes sense to the client so it should not be networked.
      <a-entity id="avatar" networked=""template:#avatar-template> // The robot's position in the scene should make sense relative to other players in the scene as well. 
      </a-entity>
    </a-entity>
  </a-scene>

Copy link
Member

Choose a reason for hiding this comment

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

For your side note, you need to call NAF.utils.takeOwnership(yourEntity) to be the owner of it so the changes propagate to the other users.
Ok if you have a different platform height for each user, your example indeed is fine, and of course you don't want it networked.
What I wanted to understand is why you need a platform height different for each user.
From your video I really don't know what I'm seeing.
But I can see that it can probably be useful if you set the platform height corresponding to an anchor on a table in AR for example, although you could just set the position on a-scene I think.
It would be interesting to have an example with one user on desktop and another with Chrome Android with AR on the phone moving a character like your video.

Copy link
Author

@Agupta00 Agupta00 Mar 16, 2023

Choose a reason for hiding this comment

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

Great, so just to confirm, I will plan to make the example with platforms of different heights then if that's okay with you?

You can play around with it after and try out your suggestions if you still have any doubts 😄 .

Copy link
Member

Choose a reason for hiding this comment

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

Yes please go ahead.
I don't know if you have an ARCore compatible Android phone. If you do, you can look at creating an experience in AR to place the platform on a table, see https://github.com/AdaRoseCannon/aframe-xr-boilerplate for an example. Chrome Android is only implementing hittest, that's enough to have the correct height of the table. Out of scope here, but I think you can now detect a plane on Meta browser on Quest, so probably you can know the table boundaries and set the width and height of the platform accordingly too.

@PlumCantaloupe
Copy link

PlumCantaloupe commented Mar 27, 2023

This is something I wrestled with also as I wanted to be able to parent and unparent networked objects to the camera dynamically.

A PR I put out was rejected a couple of years ago, but I still use and update a fork of NAF that allows for a "syncWorldTransforms" property for the networked component for this purpose, as can be seen here: #223

Most recent fork here: https://github.com/PlumCantaloupe/networked-aframe/tree/feature-add-world-sync

@Agupta00
Copy link
Author

Updated readme with video of the new example html as well.

@Agupta00 Agupta00 requested a review from vincentfretin March 30, 2023 18:28
@vincentfretin
Copy link
Member

Just to let you know, I didn't forget your PR. I was busy with other things. I'll take a deeper look at your changes and do some tests myself when I'll get the chance.

@Agupta00
Copy link
Author

Just to let you know, I didn't forget your PR. I was busy with other things. I'll take a deeper look at your changes and do some tests myself when I'll get the chance.

Np! And thanks for your reply :)

@travaglinit
Copy link

Is this PR/fork the recommended approach for nesting networked entities beneath a non-networked parent? I am looking to implement something similar where different clients might set different positions for the non-networked parent that has networked children

@vincentfretin
Copy link
Member

@travaglinit You can test the changes and give feedback on it. I didn't took the time yet to review it again and experiment on my side, sorry. I'll probably do that when I'll play with the new anchored component from aframe 1.5.0 and Quest 3.

Copy link

@travaglinit travaglinit left a comment

Choose a reason for hiding this comment

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

Added a couple comments, but nothing that would lead to breaking changes, so approving this PR.

@@ -53,6 +53,10 @@ <h1>Networked-Aframe Examples</h1>
<a href="basic-events.html">Basic with events</a>
<span>Example of listening to and logging NAF events</span>
</div>
<div>
<a href="non-networked-paraent.html">Non Networked Parent</a>

Choose a reason for hiding this comment

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

Should be "non-networked-parent.html"

@@ -53,6 +53,10 @@ <h1>Networked-Aframe Examples</h1>
<a href="basic-events.html">Basic with events</a>
<span>Example of listening to and logging NAF events</span>
</div>
<div>
<a href="non-networked-paraent.html">Non Networked Parent</a>
<span>Demonstrates how to attach networked entities as childeren of non-networked parents</span>

Choose a reason for hiding this comment

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

Should be "entities as children"

Comment on lines +159 to +160
<!-- We add allowNonNetworkedParent because we want networked entities to end up as a child of platform entity not the root a-scene.
-->

Choose a reason for hiding this comment

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

I like that we've added this documentation as comments. I would recommend also adding additional information visible in the UI of this example explaining what this example is and how to use it without needing to inspect the source code. See the "Persistent spheres" example which includes the header at the top as an example.

if (this.hasEntity(parentId)) {
this.addEntityToParent(entity, parentId);
} else if (nonNetworkedParent) {

Choose a reason for hiding this comment

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

I would recommend setting line 135 to just var nonNetworkedParent; and then the assignment for nonNetworkedParent could instead happen on this line as:

else if ((nonNetworkedParent = document.querySelector(#${parentId})))

That way we can avoid running the querySelector call if we enter the first if condition.

@travaglinit
Copy link

@vincentfretin sounds good. I reviewed the PR and added a couple of small comments, but nothing major so this PR should be good. I'll send a PR later in a different fork that includes the changes I mentioned.

I've tested the example @Agupta00 added, and it works as expected. I also re-tested all of the other examples and those are still working as expected, and the unit tests are passing.

@travaglinit
Copy link

@vincentfretin sounds good. I reviewed the PR and added a couple of small comments, but nothing major so this PR should be good. I'll send a PR later in a different fork that includes the changes I mentioned.

I've tested the example @Agupta00 added, and it works as expected. I also re-tested all of the other examples and those are still working as expected, and the unit tests are passing.

I've sent the PR from my fork which addresses the PR comments:

#440

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

Successfully merging this pull request may close these issues.

4 participants