ExtendRealityLtd / VRTK

An example of how to use the Tilia packages to create great content with VRTK v4.
https://www.vrtk.io/
MIT License
3.69k stars 993 forks source link

Teleporter will still move to an invalid location if the floor below it is valid. #2012

Closed NilsBernatzki closed 5 years ago

NilsBernatzki commented 5 years ago

Environment

VRTK v4 beta master commit hash: 17d2e9cbdf5cb5a7931cfac916d481eebd7073d4 Unity 2018.3 Vive

Steps to reproduce

  1. Follow the academy guide until the straight pointers and the teleporter prefabs have been placed and hooked up in the scene
  2. Create an Empty GameObejct to hold an IRule component and link the chosen IRule to the TargetValidity of the TeleporterFacade

--> Let's say we added an AnyTagRule (with a StringObservableList with one Element ["NonTeleport"]) and negated the AnyTagRule with a NegationRule

  1. Create a Cube and prepare it to match the chosen IRule

--> In this case select the Tag "NonTeleport"

  1. Add a Ground with a collider to the scene that doesn't share the same Tag with the earlier created Cube and move it so it is lower than the Cube

Expected behavior

With the NegationRule that inverts the Result of the AnyTagRule I would expect the TeleporterFacade to not teleport the PlayArea towards the tagged Cube once it is hovered over and selected with the pointer. I would simply expect that nothing would happen.

Current behavior

Because of the Ground object that is not tagged with "NonTeleport" the Teleporter kind of claims the Ground surface as the closest valid downwards of the collisionPoint of the EventData of the pointer, overrides the target position and teleports at this position. You end up standing inside the Cube.

Comment

In the ExampleFarmScene there are also Surfaces that cant be teleported. In most cases you added a Component to tag the objects and observe them in a ComponentTypeObservableList and use the list in a ComponentTypeRule. This Rule is passed into the TargetValidity of the PointerFacade AND the TelepoterFacade. This makes those surfaces inValid to the Pointer and therefore can't be teleported to. I think this is also the case when there is no Rule in the TeleporterFacade referenced, which makes the TagetValidity of the TeleporterFacade obsolete.

I should mention that I came across this issue while using a straight pointer. In the farm scene yo are using curved pointers for teleporting, what kind of covers up the problem I see.

But what about a surface I want to use the PointerEvents on, like (Enter/Hover/Select), but also want to block the Teleportation that will occur on selecting those surfaces? Think about a selfmade "UI" object that uses the Pointer events but its layer (e.g. "UI") is excluded from valid teleportation through a negated AnyLayerRule in the TeleporterFacades TargetValidity. Or am I missing an option in the SurfaceLocator components of the Teleporter to disable this sort of "AutoSearchNewValidSurface" - behaviour?

Thank you and greetings!

thestonefox commented 5 years ago

I think I see the issue.

The SurfaceLocator digests the rule and then when you give it a point to teleport to it looks for the nearest surface below that point and teleports you to that surface.

Now what the rule does on the Surface locator is says only match with surfaces that are valid, if the first found surface does not pass the rule then it continues searching until it finds a valid surface below (doing a RayCastAll)

This is where the problem lies because to do what you want to do, all you "should" need to do is place a zone underneath your UI object that is tagged with the "do not teleport here" info. so when the pointer hits the UI object it will tell the Teleporter to try and teleport to the surface below that point.

But what's happening is if you place the fake floor, it will simply ignore that floor and then go find the floor underneath (which is probably wanting to be teleportable) and therefore saying "i've found a valid surface"

image

So in that example, the pointer hits the blue box (which is a valid location for the pointer), the teleporter takes that point and searches for the nearest available floor, it hits the red (1) and says this is not valid, but then continues to search down for the next available surface and finds the grey floor (2) and that is valid so it moves you to that location.

Not sure of the best approach to resolving this yet, because the current logic still is sound because you may want an object to be ignored.....

But thinking about it, if you have a scenario like:

image

Then the surface locator should be ignoring the red floor not by rule but instead by the layer it is put upon (e.g. IgnoreRaycasts)

I'm sure there was a reason we had it do the FindAllCollisions logic but I can't think off the top of my head why. @bddckr do you have any recollection of why we did it that way?

thestonefox commented 5 years ago

Ok another idea. The SurfaceLocator could have another rule on top of TargetValidity like CastTerminator

And any surface set up with a rule that passes the CastTerminator would then also cancel the RayCastAll.

So then you could have this sort of set up:

image

Where a TargetValidity rule will let the surface locator ignore a target and continue to cast down looking for the nearest floor, but if the cast hits an object that matches the CastTerminator rule then it terminates the RayCastAll from continuing the surface checks.

The default behaviour would be still to have your pointer preempt ignoring teleport locations so no change to the facade would be required as it would still operate as expected.

But if you wanted the outcome you're after then you could place a slim collider under the UI object that is marked as an invalid teleport location but also marked as a CastTerminator.

NilsBernatzki commented 5 years ago

The CastTerminatorsetup sounds promissing... Does that mean that I would simply reference a new set of Rules into the CastTerminator field of a TeleporterFacade to stop the SurfaceLocator from searching once it notices a match? That would definetly do it.

Like this?

CastTerminator

Meanwhile I'll try an implementation that recieves the ObjectPointer.EventData and pipes the result into the pattern of defining a set of rules with marked GameObjects as "inValidForTeleport" (e.g. by Tag/Layer/Component). Once the Rules match with a target, the object emits an EmptyEvent on Enter and Exit. I could use that to disable/enable the TeleporterFacade Prefab so the SelectionAction won't trigger the Teleport() function. Not as smart as the CastTerminator approach, because that means I can't teleport with a one controller while hovering over an "inValidForTeleport" object with the other, but to prevent conflicts until the issue is resolved, it will do :)

thestonefox commented 5 years ago

So yea, the CastTermination would be a new rule set on the SurfaceLocator. You'd probably need to reference them on the numerous SurfaceLocators rather than on the TeleporterFacade otherwise it could be a bit confusing. I've not fully thought it through yet, but the theory should work.

As for trying to pipe the ObjectPointer data. The TeleportFacade receives a TransformData which is the point in space where you want to teleport to.

That TransformData contains a Transform which I'm not sure if that is the transform of the cursor of the pointer. If it is then doing any logic on the Teleport data may be frivolous.

bddckr commented 5 years ago

I'm sure there was a reason we had it do the FindAllCollisions logic but I can't think off the top of my head why. @bddckr do you have any recollection of why we did it that way?

We can't just stop at the first one that is invalid in a lot of the usages of this feature as that would mean users have no way to allow to continue the search. The proposed additional Rule would bring the needed three-value-behavior: Ignore vs. Stop vs. Use the hit object.

thestonefox commented 5 years ago

Here are 3 cases to confirm the new Rule concept

image

Case 1: Point at the blue surface that can digest pointer, but Teleporter tries to locate nearest surface and the orange collider underneath has a CastTermination rule on it so there's no valid teleport location.

Case 2: The floor underneath the blue surface is a valid teleport location and because the orange blocker is higher then any teleportation to that floor can still occur.

Case 3: This is the reason why you would want the rule on the SurfaceLocator, there are 2 Surface Locators in a teleporter, the one that locates the nearest floor from the point to teleport to, and the one from the headset that is constantly doing the Snap To Floor. In this case the Teleport SurfaceLocator would be ignoring the orange collider so any teleport beam would not allow teleportation, however the headset SnapToFloor surface locator wouldn't have the CastTermination rule set up so it would still consider the orange blocker to be a target to ignore and then still find the nearest floor (the grey surface below) and still successfully teleport you back to the ground.

NilsBernatzki commented 5 years ago

Great! I think I got it now :) Didn't know about the SurfaceLocator on the headset.

thestonefox commented 5 years ago

PR with proposed solution https://github.com/ExtendRealityLtd/Zinnia.Unity/pull/392

Will go into Zinnia first then merged into VRTK afterwards

NilsBernatzki commented 5 years ago

What happened? Isnt't the issue resolved by https://github.com/ExtendRealityLtd/Zinnia.Unity/pull/392 ?

thestonefox commented 5 years ago

That fix in zinnia hasn't been brought into vrtk yet so the issue will still exist until then