Implement rotate and scale button functions with bitecs #5993

takahirox · 2023-03-11T03:46:27Z

This commit adds the implementations of Object menu rotate and scale button functions.

Note that this commit focuses on making them just functional with minimal change.

The implementation relys on the old systems to save development time. It should be rewritten more elegantly to fit more to bitECS.

The main logic of src/components/scale-button is exposed to be reused from the new and old systems.

takahirox · 2023-04-18T22:25:53Z

Ready for review.

takahirox · 2023-04-18T22:52:07Z

src/components/scale-button.js

+
+AFRAME.registerComponent("scale-button", {
+  init() {
+    this.handler = new ScalingHandler(this.el.object3D, this.el.sceneEl.systems["transform-selected-object"]);


I'm not familiar with A-Frame component/system lifecycles. Is it safe to assume that transform-selected-object system is already registered here?

I think it should per:
https://github.com/mozilla/hubs/blob/3c897c0461fca5591646ce531af0a76c2cef3f32/src/hub.js#L123-L124

Thanks.

And if I'm right, system seems to be registered to sceneEl.systems when AFRAME.registerSystem is called (when a file is imported) while Component.init() seems to be called later (when the component first initialized?). So probably it is safe to assume here.

keianhzo

LGTM, just a few nits. The menu placement improvements can probably be addressed in a follow-up.

keianhzo · 2023-04-19T10:20:54Z

src/bit-systems/object-menu.ts

@@ -1,7 +1,15 @@
-import { defineQuery, entityExists, hasComponent } from "bitecs";
-import { Matrix4, Vector3 } from "three";
+import { addComponent, defineQuery, enterQuery, entityExists, exitQuery, hasComponent } from "bitecs";


addComponent and path import are not used anymore

Thanks, updated.

keianhzo · 2023-04-19T10:25:57Z

src/bit-systems/object-menu.ts

 const _mat4 = new Matrix4();

+// TODO: Avoid any


Why can't this be of type ScalingHandler?

let scalingHandler: ScalingHandler | null = null;

Good catch, updated.

keianhzo · 2023-04-19T10:27:25Z

src/bit-systems/object-menu.ts

@@ -39,10 +54,69 @@ function moveToTarget(world: HubsWorld, menu: EntityID) {
  const targetObj = world.eid2obj.get(ObjectMenu.targetRef[menu])!;
  targetObj.updateMatrices();

+  // TODO: position the menu more carefully...


We should probably do this before removing the newLoader flag, the menu interesects with the objects pretty easily. We can do it in follow-up PR.

We can do it in follow-up PR.

Yes, that's the plan.

We should probably just aim for roughly what we have now in terms of menu positioning, we still would like to ultimately not have these in world object menus in their current form on most platforms. On desktop and mobile we likely end up with more traditional 2d context menus, and in VR we likely don't need most of what the menus do since direct manipulation is perdurable (we still need to figure out what to do with the rest of the options)

keianhzo · 2023-04-19T10:27:42Z

src/bit-systems/object-menu.ts

+}
+
+// TODO: startRotation/Scaling() and stopRotation/Scaling() are
+//       temporarl implementation that rely on the old systems.


temporarl -> temporary

Thanks, updated.

keianhzo · 2023-04-19T10:36:45Z

src/bit-systems/object-menu.ts

+  // TODO: Don't use any
+  // TODO: Remove the dependency with AFRAME


If you want avoid using any and avoid using the AFRAME symbol you can also use:

APP.scene!

and the add the type to the aframe.d.ts:

interface TransformSelectedObjectSystem extends ASystem { startTransform(Object3D, EntityID, any): void; stopTransform(): void; } interface AScene extends AElement { object3D: Scene; renderStarted: boolean; renderer: WebGLRenderer; tick(time: number, delta: number): void; isPlaying: boolean; behaviors: { tick: AComponent[]; tock: AComponent[]; }; systemNames: Array<keyof AScene["systems"]>; systems: { "hubs-systems": HubsSystems; "personal-space-bubble": PersonalSpaceBubbleSystem; "transform-selected-object": TransformSelectedObjectSystem; userinput: UserInputSystem; /** @deprecated see bit-interaction-system */ interaction: InteractionSystem; nav: NavSystem; }; emit(string, any?): void; addState(string): void; is(string): boolean; }

There are some other bitecs systems that have AFRAME dependency. I hope we will work on it when we remove the dependency.

Agree with Manuel that just adding the type to the aframe.d.ts would be nice. Its fairly minor either way but prevents from making some accidental mistakes leaking the any around.

OK, two developers suggested the same thing. And the change is minor. Updated.

keianhzo · 2023-04-19T10:37:41Z

src/bit-systems/object-menu.ts

+  // TODO: Don't use any
+  // TODO: Remove the dependency with AFRAME


keianhzo · 2023-04-19T10:38:05Z

src/bit-systems/object-menu.ts

+  scalingHandler.startScaling(world.eid2obj.get(rightCursorEid));
+}
+
+function stopScaling(world: HubsWorld, targetEid: EntityID) {


targetEid doens't seem to be needed anymore.

Good catch, updated.

takahirox · 2023-04-25T22:19:03Z

This PR needs to be unlocked by the reviewers to get merged.

netpro2k

This looks good. Made a few comments but most are just observations or thoughts I had while reviewing and not really suggestions for changes.

netpro2k · 2023-04-28T18:32:48Z

src/bit-systems/object-menu.ts

@@ -39,10 +54,69 @@ function moveToTarget(world: HubsWorld, menu: EntityID) {
  const targetObj = world.eid2obj.get(ObjectMenu.targetRef[menu])!;
  targetObj.updateMatrices();

+  // TODO: position the menu more carefully...


We should probably just aim for roughly what we have now in terms of menu positioning, we still would like to ultimately not have these in world object menus in their current form on most platforms. On desktop and mobile we likely end up with more traditional 2d context menus, and in VR we likely don't need most of what the menus do since direct manipulation is perdurable (we still need to figure out what to do with the rest of the options)

netpro2k · 2023-04-28T18:35:37Z

src/bit-systems/object-menu.ts

+  // TODO: Don't use any
+  // TODO: Remove the dependency with AFRAME


Agree with Manuel that just adding the type to the aframe.d.ts would be nice. Its fairly minor either way but prevents from making some accidental mistakes leaking the any around.

netpro2k · 2023-04-28T18:38:41Z

src/bit-systems/object-menu.ts

+  // TODO: Remove the dependency with AFRAME
+  const transformSystem = (AFRAME as any).scenes[0].systems["transform-selected-object"];
+  const physicsSystem = AFRAME.scenes[0].systems["hubs-systems"].physicsSystem;
+  physicsSystem.updateBodyOptions(Rigidbody.bodyId[targetEid], { type: "kinematic" });


Not something to address here but just to get it bouncing around in everyone's head. We change these physics properties pretty arbitrarily all over and its very easy for things to end up in weird states and to overwrite one system's desired physics settings with another. We need to think of how to manage that. It mostly comes about when something wants to set the position/rotation of a physics object since it must be kinematic or static to do so, but then it needs to be able to return to its previous state.

netpro2k · 2023-04-28T18:43:44Z

src/bit-systems/object-menu.ts

    handleClicks(world, menu, hubChannel);
+
+    heldExitQuery(world).forEach(eid => {


My first thought would have been to do hasComponent(world, Held, ObjectMenu.rotateButtonRef[menuEid) comparison to the current transform system state.

Using quires is fine, and maybe its nice that the previous frame state is handled by the enter/exit queries but just noting. Not sure which I like better.

I can understand.

Using hasComponent(world, Held, ObjectMenu.rotateButtonRef[menuEid]) may be more efficient because Held component may be a common Component so heldQuery might include a way more entity ids than needed here.

But it required to manage previous frame state. I want to avoid to manage a previous frame state in a system for simplicity.

Using queries may be inefficient but we don't need to manually manage previous frame state so it is simpler.

I want to go as is for now. We may revisit later if we face performance problem.

netpro2k · 2023-04-28T18:47:02Z

src/prefabs/object-menu.tsx

@@ -198,6 +199,8 @@ function ScaleButton(props: Attrs) {
      height={buttonHeight}
      type={BUTTON_TYPES.ACTION}
      text={"scale"}
+      holdable
+      holdableButton


Bit weird to have to specify holdableButton on a Button3D should probably be handled in Button3D based on just holdable.

The disconnect between things being components and things just being properties to a JSX function here is definitely something I don't love, but its just how JSX works. Like here "height" is not a component nor is "text".

I think mixing components "holdable" and "holdableButton" into it makes it even more confusing.

Updated to remove holdableButton here.

netpro2k · 2023-04-28T18:53:30Z

src/components/scale-button.js

-      if (!(NAF.utils.isMine(this.networkedEl) || NAF.utils.takeOwnership(this.networkedEl))) {
-        return;
-      }
+  startScaling(object3D) {


This is temporary transition code so it doesn't really matter much, but we might try to name this parameter a bit better. Maybe cursorObj? interactor?

netpro2k · 2023-04-28T18:56:43Z

src/bit-systems/object-menu.ts

+}
+
+function stopScaling(world: HubsWorld) {
+  const rightCursorEid = anyEntityWith(world, RemoteRight)!;


Not a suggestion for here but reminds me of an observation I want us to think about.

I find it annoying that we don't have a good way to statically define references like this and have to "look them up" every frame with a query like this. Its not particularly expensive or anything, just silly because we actually know what number entity this is going to be every time. The only thing really stopping us from doing it is module loading order.

I don't love anyEntityWith(). Added this topic in the internal design document.

This commit adds the implementations of Object menu rotate and scale button functions. Note that this commit focuses on making them just functional with minimal change. The implementation relys on the old systems to save development time. It should be rewritten more elegantly to fit more to bitECS. The main logic of src/components/scale-button is exposed to be reused from the new and old systems.

takahirox had a problem deploying to hc-bio March 11, 2023 03:52 — with GitHub Actions Failure

takahirox had a problem deploying to smoke March 11, 2023 03:52 — with GitHub Actions Failure

takahirox force-pushed the BitECSRotateAndScaleButtons branch from df0c441 to 8ce57a6 Compare March 14, 2023 00:41

takahirox had a problem deploying to hc-bio March 14, 2023 00:48 — with GitHub Actions Failure

takahirox had a problem deploying to smoke March 14, 2023 00:48 — with GitHub Actions Failure

takahirox mentioned this pull request Mar 14, 2023

BitECS Component Burndown #5899

Open

50 tasks

takahirox force-pushed the BitECSRotateAndScaleButtons branch from 8ce57a6 to 48863f4 Compare April 14, 2023 22:50

takahirox had a problem deploying to hc-bio April 14, 2023 23:05 — with GitHub Actions Failure

takahirox had a problem deploying to smoke April 14, 2023 23:05 — with GitHub Actions Failure

takahirox force-pushed the BitECSRotateAndScaleButtons branch from 48863f4 to 3c897c0 Compare April 18, 2023 22:22

takahirox marked this pull request as ready for review April 18, 2023 22:24

takahirox had a problem deploying to hc-bio April 18, 2023 22:28 — with GitHub Actions Failure

takahirox had a problem deploying to smoke April 18, 2023 22:28 — with GitHub Actions Failure

takahirox requested review from johnshaughnessy, netpro2k and keianhzo April 18, 2023 22:44

takahirox commented Apr 18, 2023

View reviewed changes

keianhzo requested changes Apr 19, 2023

View reviewed changes

takahirox had a problem deploying to smoke April 20, 2023 23:04 — with GitHub Actions Failure

takahirox had a problem deploying to hc-bio April 20, 2023 23:04 — with GitHub Actions Failure

takahirox force-pushed the BitECSRotateAndScaleButtons branch from 3f33f1a to 326fb66 Compare April 20, 2023 23:09

takahirox had a problem deploying to smoke April 20, 2023 23:16 — with GitHub Actions Failure

takahirox had a problem deploying to hc-bio April 20, 2023 23:16 — with GitHub Actions Failure

netpro2k reviewed Apr 28, 2023

View reviewed changes

takahirox force-pushed the BitECSRotateAndScaleButtons branch 2 times, most recently from 96bc1c1 to a76fef2 Compare May 1, 2023 19:07

takahirox had a problem deploying to hc-bio May 1, 2023 19:14 — with GitHub Actions Failure

takahirox had a problem deploying to smoke May 1, 2023 19:14 — with GitHub Actions Failure

keianhzo approved these changes May 11, 2023

View reviewed changes

takahirox force-pushed the BitECSRotateAndScaleButtons branch from a76fef2 to ee1eeed Compare May 11, 2023 21:58

takahirox force-pushed the BitECSRotateAndScaleButtons branch from ee1eeed to aa42562 Compare May 11, 2023 22:01

takahirox had a problem deploying to smoke May 11, 2023 22:07 — with GitHub Actions Failure

takahirox had a problem deploying to hc-bio May 11, 2023 22:07 — with GitHub Actions Failure

takahirox merged commit b2136ea into master May 11, 2023

takahirox deleted the BitECSRotateAndScaleButtons branch May 11, 2023 22:12

takahirox mentioned this pull request May 11, 2023

Fix bitECS Object menu system #6075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement rotate and scale button functions with bitecs #5993

Implement rotate and scale button functions with bitecs #5993

takahirox commented Mar 11, 2023 •

edited

Loading

takahirox commented Apr 18, 2023

takahirox Apr 18, 2023

keianhzo Apr 19, 2023

takahirox Apr 20, 2023

keianhzo left a comment

keianhzo Apr 19, 2023

takahirox Apr 20, 2023

keianhzo Apr 19, 2023

takahirox Apr 20, 2023

keianhzo Apr 19, 2023

takahirox Apr 20, 2023

netpro2k Apr 28, 2023

keianhzo Apr 19, 2023

takahirox Apr 20, 2023

keianhzo Apr 19, 2023

takahirox Apr 20, 2023

netpro2k Apr 28, 2023

takahirox May 1, 2023

keianhzo Apr 19, 2023

keianhzo Apr 19, 2023

takahirox Apr 20, 2023

takahirox commented Apr 25, 2023

netpro2k left a comment

netpro2k Apr 28, 2023

netpro2k Apr 28, 2023

netpro2k Apr 28, 2023

netpro2k Apr 28, 2023

takahirox May 1, 2023

netpro2k Apr 28, 2023

takahirox May 1, 2023

netpro2k Apr 28, 2023

netpro2k Apr 28, 2023

takahirox May 1, 2023

		// TODO: Don't use any
		// TODO: Remove the dependency with AFRAME

		handleClicks(world, menu, hubChannel);

		heldExitQuery(world).forEach(eid => {

Implement rotate and scale button functions with bitecs #5993

Implement rotate and scale button functions with bitecs #5993

Conversation

takahirox commented Mar 11, 2023 • edited Loading

takahirox commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keianhzo left a 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

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

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

takahirox commented Apr 25, 2023

netpro2k left a 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

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

takahirox commented Mar 11, 2023 •

edited

Loading