-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
New Entity Framework #5536
New Entity Framework #5536
Conversation
Co-authored-by: Dominick D'Aniello <[email protected]>
Co-authored-by: Dominick D'Aniello <[email protected]>
@@ -9,7 +10,8 @@ export const Layers = { | |||
CAMERA_LAYER_VIDEO_TEXTURE_TARGET: 5, | |||
|
|||
CAMERA_LAYER_THIRD_PERSON_ONLY: 6, | |||
CAMERA_LAYER_FIRST_PERSON_ONLY: 7 | |||
CAMERA_LAYER_FIRST_PERSON_ONLY: 7, | |||
CAMERA_LAYER_UI: 8 |
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.
This was introduced to simplify camera tool code, but is also something we want for other reasons in the future (namely being able to isolate UI from tonemapping after we add post effects)
@@ -19,35 +21,22 @@ export const Layers = { | |||
*/ | |||
AFRAME.registerComponent("layers", { | |||
schema: { |
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.
We technically had this exposed in gltf-component-mappings but it is not used by Spoke or the Blender Exporter so it is highly unlikely anyone would have used it for anything. Simplifying the schema to more directly map to the counterpart in BitECS land.
}, | ||
|
||
remove() { | ||
this.geometry?.dispose(); |
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.
We were previously leaking this geometry. I noticed it while making sure to clean up the geometry in the new system.
@@ -279,7 +279,7 @@ AFRAME.registerSystem("transform-selected-object", { | |||
.applyQuaternion(q.copy(plane.quaternion).invert()) | |||
.multiplyScalar(SENSITIVITY / cameraToPlaneDistance); | |||
if (this.mode === TRANSFORM_MODE.CURSOR) { | |||
const modify = AFRAME.scenes[0].systems.userinput.get(paths.actions.transformModifier); | |||
const modify = !AFRAME.scenes[0].systems.userinput.get(paths.actions.transformModifier); |
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.
This changes the default for object rotation to unsapped. This was useful for the camera tool, and is something we have said we should do for a long time, so I decided not to make it conditional.
import { createElementEntity } from "./utils/jsx-entity"; | ||
/** @jsx createElementEntity */ createElementEntity; | ||
|
||
AFRAME.GLTFModelPlus.registerComponent("media-frame", "media-frame", (el, _componentName, componentData) => { |
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.
Ultimately GLTF loading will eventually go entirely through the new system of inflators, but for now just exposing media frames here is useful, even if slightly awkward.
@@ -122,6 +123,14 @@ export default class MessageDispatch extends EventTarget { | |||
this.scene.systems["hubs-systems"].soundEffectsSystem.playSoundOneShot(SOUND_QUACK); | |||
} | |||
break; | |||
case "cube": { |
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.
This will not be a "supported" feature and will likely go away in the future but is a useful way of testing the new networking system.
@@ -168,11 +168,10 @@ export default class PhoenixAdapter { | |||
|
|||
message.source = source; | |||
|
|||
//TODO: Handle frozen | |||
if (this.frozen) { | |||
if (this.frozen && (message.dataType === "um" || message.dataType === "u")) { |
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.
We were previously storing away messages that were not u
or um
(like the pen drawings) but acting as if they were update messages... This likely caused some of the odd behavior we saw with drawings. The new networking system also runs over one of these custom message types and currently just ignores freeze state
|
||
if (myCamera) { | ||
myCamera.parentNode.removeChild(myCamera); | ||
const myCam = anyEntityWith(APP.world, MyCameraTool); |
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.
Note the concept of what "my camera" is was intentionally simplified. Previously if someone grabbed a camera you spawned we (for some purposes) considered it to be "their" camera (though it would still get deleted if you left the room. Now you can only spawn 1 camera, and that one is always "yours". To remove it you click the button (or press c) again. The camera has no object menu.
if (!ents.length) return; | ||
|
||
// Check only one per frame | ||
const eid = ents[world.time.tick % ents.length]; |
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.
Doing all the work in 1 spot eliminates the need for something like our "frame scheduler" we have. Instead you just do only exactly the work you want to do each frame.
@@ -26,6 +26,10 @@ export function disposeMaterial(mtrl) { | |||
if (mtrl.normalMap) mtrl.normalMap.dispose(); | |||
if (mtrl.specularMap) mtrl.specularMap.dispose(); | |||
if (mtrl.envMap) mtrl.envMap.dispose(); | |||
if (mtrl.aoMap) mtrl.aoMap.dispose(); |
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.
We were previously leaking these. Noticed while working on object cleanup for the new system
return ref.current; | ||
} | ||
|
||
export function createElementEntity(tag, attrs, ...children) { |
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.
This is the function JSX transforms to:
<entity foo={{bar: 1}}>
<entity biz />
</entiti>
translates to:
createElementEntity("entity", {foo: {bar: 1}}, [
createElementEntity("entity", {biz: true}}, null)
])
For the name, I was going for something like React.createElement
but I think this name is confusing. Open to suggestions.
return key; | ||
}; | ||
|
||
// TODO this array encoding is silly, use a buffer once we are not sending JSON |
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.
This is about as noisy as our current JSON encoding in NAF... It should be quite easy to make this much better when we move to a binary socket.
@@ -105,7 +114,7 @@ export class AppAwareTouchscreenDevice { | |||
// If grab was being delayed, we should fire the initial grab and also delay the unassignment | |||
// to ensure we write at least two frames with the grab down (since the action set will change) | |||
// and otherwise we'd not see the falling xform. | |||
if (assignment.framesUntilGrab >= 0) { | |||
if (assignment.framesUntilGrab > 0) { |
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 actually don't have a good idea of why this fix was needed (and if it was even a regression). Without it I noticed that sometimes objects would stick to the cursor and never be dropped)
|
||
if ((!this.clickedOnAnything && buttonLeft) || buttonRight) { | ||
if ((buttonRight && !transformSystem.transforming) || (buttonLeft && !anyEntityWith(APP.world, HeldRemoteRight))) { |
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.
This got suspiciously simple... I am still not 100% convinced some functionality was not lost here... But everything seems to be working correctly.
This PR introduces the foundations for the new framework for writing core "client engine" code in Hubs.
To prove things out this PR ports the media frame and the camera tool functionality. These are now built entirely without using any AFRAME entities/components/systems and using a new networking system. This also required rewriting the interactions system as it needed to be able to inter op with both new and legacy entities.
This is just the beginning steps and things will likely be changing quite a bit in the near term while we continue to flesh things out. More documentation and examples will happen after things settle down into a more steady state.
Motivations
Goals
Non-Goals
High Level Plan
Some things worth noting
object3d
property on<entity/>
tags is an escape hatch that potentially breaks these semantics, and likely will be phased out as we build out the core library of components.Known TODOs
There are many inline TODOs in the new code introduced in this PR. None of them are worth holding back merging these changes but a few are worth noting more directly:
Camera Tool
Netcode
Media Frames
Pre Merge TODOS