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

More bit gltf components #5894

Merged
merged 8 commits into from
Jan 13, 2023
Merged

More bit gltf components #5894

merged 8 commits into from
Jan 13, 2023

Conversation

netpro2k
Copy link
Contributor

This PR implements skybox, fog, background, shadow, spawn-point and spawner components in BitECS. Most of them are considered deprecated, but are needed for loading many existing scenes.

The spawner component still needs a bit of work to be grabbable with VR hands, but this requires deeper work on the physics integration into BitECS so leaving that for a later PR.

src/bit-systems/object-spawner.ts Show resolved Hide resolved
src/bit-systems/object-spawner.ts Outdated Show resolved Hide resolved
const spawnerExitQuery = exitQuery(defineQuery([ObjectSpawner]));
export function objectSpawnerSystem(world: HubsWorld) {
interactedSpanersEnterQuery(world).forEach(spawner => {
if (!jobs.has(spawner)) jobs.set(spawner, coroutine(spawnObjectJob(world, spawner)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to use the same language to describe these things across all the coroutine-powered systems:

  • jobs : Map<EntityID, Coroutine>()
  • function* doTheThingJob (instead of function* doTheThing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ritual of each system managing these also feels a bit silly.

src/inflators/environment-settings.ts Outdated Show resolved Hide resolved
src/inflators/environment-settings.ts Show resolved Hide resolved
@@ -63,7 +63,20 @@ export function inflateModel(world: HubsWorld, rootEid: number, { model }: Model
const components = obj.userData.gltfExtensions?.MOZ_hubs_components || {};
if (components.visible) {
const { visible } = components.visible;
obj.visible = !!visible;
obj.visible = visible;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this value validated to be true or false (and never something like 0)? (I don't remember why we wrote !!visible but we may want a non-boolean value to throw or be converted to false.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The visible component has 1 property visible which is a boolean. We technically don't validate component schemas anywhere, but if we want to it should probably be upstream from here.

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

// TODO rigidbody+shape for hand grabbing
Copy link
Contributor

Choose a reason for hiding this comment

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

We may encounter the moveTheParentNotTheMesh problem when we do this, since that had to do with where the mesh was relative to the object root (and needing the physics shape to be centered around the object's center-of-mass) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah perhaps. It should hopefully be much more straightforward though. We also may be able to avoid that whole class of problem if we rethink physics shapes a bit.

const nextFramePromise = Promise.resolve();
export function nextFrame() {
return nextFramePromise;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The name nextFrame is a little weird to me, since it assumes information about the calling context (i.e. this coroutine that ticks every frame). It's accurate everywhere we've wanted to use it, but still weird to me. I don't have a better name in mind though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. This is part of the "courtine API" like timers stuff we experimented with so I guess I will add cr. I don't like that much either but at least its consistent. Maybe we want to do something like Coroutine.nextFrame()

}

export const schemas: Map<Component, NetworkSchema> = new Map();
schemas.set(NetworkedMediaFrame, NetworkedMediaFrameSchema);
schemas.set(NetworkedTransform, NetworkedTransformSchema);
schemas.set(NetworkedVideo, NetworkedVideoSchema);
schemas.set(NetworkedWaypoint, NetworkedWaypointSchema);
schemas.set(NetworkedFloatyObject, {
componentName: "floaty-object",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be networked-floaty-object? (I guess the networked- prefix might be implied... I wasn't as clever with any of the other ones so they all have networked- in their names.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinions here. Would like to get rid of this somehow as its just another thing you may mess up on copy/paste when making a new component (I almost screwed up matching the two instances of NetworkedFloatyObject in this block.

serializeForStorage: (eid: EntityID) => StoredComponent;
deserializeFromStorage: (eid: EntityID, storedComponent: StoredComponent) => void;
serializeForStorage?: (eid: EntityID) => StoredComponent;
deserializeFromStorage?: (eid: EntityID, storedComponent: StoredComponent) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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.

2 participants