Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

Add light-test, and related changes #173

Merged
merged 3 commits into from
Feb 5, 2019
Merged

Add light-test, and related changes #173

merged 3 commits into from
Feb 5, 2019

Conversation

eanders-ms
Copy link
Contributor

The light component is one of the oldest features of our API but hasn't been used much, so it was interesting to revisit it. I found a few places where it could be improved, and these changes also applied to the other actor components: text and rigid body. In summary: I removed the enable-* payloads and instead relied on the actor patching mechanism to enable components. This was already 99% supported in the SDK, and 100% in the client, so the enable-* messages were unnecessary. This is a nice reduction in complexity! Checkout all the deleted code!

Fixes issue #99

Clip of the light test in action
light-test

Other changes:

  • The most recent tests I added follow a design pattern that allows the user to click on anything in the test to quit, or wait one minute. I updated my two previous tests to the latest version of this pattern.
  • Added a static method to Quaternion to create a "look at" rotation.
  • Updated the call signature of the observe method to be more self documenting.

return true;
} finally {
destroyActors(this.sceneRoot);
}
}
Copy link
Contributor

@sorenhan sorenhan Feb 5, 2019

Choose a reason for hiding this comment

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

managing actors shouldn't be needed anymore, as functional test menu wrapper cleans up all actors on exit. It's fine to do it manually, but I think we agreed unnecessary, right? #Pending

Copy link
Contributor

Choose a reason for hiding this comment

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

(just noting here because you added extra code around the destroy call)


In reply to: 254025155 [](ancestors = 254025155)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's right. I'll take this out.


In reply to: 254025313 [](ancestors = 254025313,254025155)

});
}).value;
label.setBehavior(MRESDK.ButtonBehavior).onClick('released', () => this.running = false);

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 label clickable wihtout having a collidable flag set anywhere? are colliders created by setbehavior, or automatically, or am I not reading this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here for the future when colliders on empty objects with text are possible. Of course by that time, we'll probably have a generalized way to exit a test and it won't be necessary.


In reply to: 254026146 [](ancestors = 254026146)

Copy link
Contributor

@sorenhan sorenhan left a comment

Choose a reason for hiding this comment

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

Love the functionality, and it's super clean. I'm not convinced about adding directional lights (as they are global) to all MREs (which by default are local, but can be used as global). I think we can check it in and restrict/lock down later.

@eanders-ms eanders-ms merged commit 2b0830c into red Feb 5, 2019
@eanders-ms eanders-ms deleted the eanders/light branch February 5, 2019 20:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants