-
Notifications
You must be signed in to change notification settings - Fork 296
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
base: master
Are you sure you want to change the base?
Conversation
src/components/networked.js
Outdated
@@ -427,6 +428,7 @@ AFRAME.registerComponent('networked', { | |||
syncData.template = data.template; | |||
syncData.persistent = data.persistent; | |||
syncData.parent = this.getParentId(); | |||
syncData.attachToParentId = data.attachToParentId; |
There was a problem hiding this comment.
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.
examples/basic.html
Outdated
@@ -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;"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄 .
There was a problem hiding this comment.
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.
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 |
Updated readme with video of the new example html as well. |
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 :) |
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 |
@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. |
There was a problem hiding this 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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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"
<!-- We add allowNonNetworkedParent because we want networked entities to end up as a child of platform entity not the root a-scene. | ||
--> |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
@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: |
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