[Dev→master] ToolTip #1936
[Dev→master] ToolTip #1936
Conversation
…, colocated properties, public enums in separate files etc.
| TopRightCorner = 6, | ||
| TopLeftCorner = 7, | ||
| // Automatic options | ||
| Center, |
There was a problem hiding this comment.
I noticed we only define some of these enums but not all. Really only the first one is required to be zero.
suggestion: Let's make ClosestCorner = 0, as the default.
There was a problem hiding this comment.
If ClosestCorner becomes 0, it will be important to also update the dev branch. otherwise apps will get an unexpected setting change when migrating
There was a problem hiding this comment.
Actually, ClosestCorner cannot be zero because the first eight enums are fixed as indices into an array.
ClosestCorner is one of a collection of runtime-computed enum options so it has to be defined after the initial values of 0-7
| /// <summary> | ||
| /// No state to have from Manager | ||
| /// </summary> | ||
| None, |
| InFront, | ||
| } | ||
|
|
||
|
|
| /// </summary> | ||
| public enum ConnectorPivotDirection | ||
| { | ||
| Manual, // Direction will be specified manually |
| /// <summary> | ||
| /// How does the Tooltip track with its parent object | ||
| /// </summary> | ||
| public enum ConnectorFollowType |
There was a problem hiding this comment.
Needs [Flags] attribute
| /// </summary> | ||
| public enum ConnectorFollowType | ||
| { | ||
| AnchorOnly, // The anchor will follow the target - pivot remains unaffected |
There was a problem hiding this comment.
Use bit assignments to do bitwise operations on the enum value.
| public enum ConnectorFollowType | ||
| { | ||
| AnchorOnly, // The anchor will follow the target - pivot remains unaffected | ||
| PositionOnly, // Anchor and pivot will follow target position, but not rotation |
There was a problem hiding this comment.
PositionOnly = 1<<1
There was a problem hiding this comment.
Let's rename this to Position
| { | ||
| AnchorOnly, // The anchor will follow the target - pivot remains unaffected | ||
| PositionOnly, // Anchor and pivot will follow target position, but not rotation | ||
| PositionAndYRotation, // Anchor and pivot will follow target like it's parented, but only on Y axis |
There was a problem hiding this comment.
Let's rename this to YRotation = 1<<2
| AnchorOnly, // The anchor will follow the target - pivot remains unaffected | ||
| PositionOnly, // Anchor and pivot will follow target position, but not rotation | ||
| PositionAndYRotation, // Anchor and pivot will follow target like it's parented, but only on Y axis | ||
| PositionAndRotation, // Anchor and pivot will follow target like it's parented |
There was a problem hiding this comment.
Let's rename this to XRotation = 1<<3
enum names, bit shifting, remove empty lines
| @@ -56,6 +56,11 @@ public ButtonStates() | |||
| public bool RaiseEventsBasedOnVisibility; | |||
| public InteractionSourceInfo SourceKind; | |||
|
|
|||
| //Navigation Gesture Emulation vars | |||
| Vector3 NavigatorValues = Vector3.zero; //holds the navigation gesture values [-1,1] | |||
There was a problem hiding this comment.
please explicitly state visibility for class variables
There was a problem hiding this comment.
changes to this file are from previous commit(s) by other developers.
Its not part of the Tooltip commit.
There was a problem hiding this comment.
ill make the change anyway because its clear what the effect would be.
There was a problem hiding this comment.
You may wish to rebase against the may18_dev branch to eliminate files that you are not modifying. the branch was updated to "final" 2017.2.1.4 changes yesterday.
Thanks for being willing to change this, though after the rebase you may not need to :)
| @@ -456,6 +480,11 @@ private void SendControllerStateEvents(float time) | |||
| else | |||
| { | |||
| InputManager.Instance.RaiseManipulationUpdated(this, controllerId, currentButtonStates.CumulativeDelta); | |||
|
|
|||
| //Navigation Gesture Emulation | |||
| NavigatorValues.x = Mathf.Clamp(currentButtonStates.CumulativeDelta.x*5, -1.0f, 1.0f) * railUsedCurrently.x; | |||
There was a problem hiding this comment.
what is the multiply by 5 accomplishing? is this something that should be configurable?
There was a problem hiding this comment.
changes to this file are from previous commit(s) by other developers.
Its not part of the Tooltip commit.
There was a problem hiding this comment.
You may wish to rebase against the may18_dev branch to eliminate files that you are not modifying. the branch was updated to "final" 2017.2.1.4 changes yesterday.
| [Tooltip("Current State of the Button")] | ||
| public ButtonStateEnum ButtonState = ButtonStateEnum.Observation; | ||
| private ButtonStateEnum buttonState = ButtonStateEnum.Observation; |
There was a problem hiding this comment.
Is this the same ButtonStateEnum used by the AppBar?
There was a problem hiding this comment.
changes to this file are from previous commit(s) by other developers.
Its not part of the Tooltip commit.
There was a problem hiding this comment.
You may wish to rebase against the may18_dev branch to eliminate files that you are not modifying. the branch was updated to "final" 2017.2.1.4 changes yesterday.
| ScaleToFitContent(); | ||
| } | ||
|
|
||
| protected abstract void ScaleToFitContent(); |
There was a problem hiding this comment.
should a default implementation be provided?
There was a problem hiding this comment.
the overrides (tooltipBackgroundMesh, tooltipBackgroundBlob, and tooltipBackgroundCorners have such different overrides for this abstract function that it really isn't possible to make a useful virtual function with default implementation. I could change the abstract prototype to a virtual function and have it be empty but that seems like it violates the thinking behind the original design.
| { | ||
| /// <summary> | ||
| /// Renders meshes at the corners of a tool tip | ||
| /// TODO implement color changes |
There was a problem hiding this comment.
remove this TODO in favor of a workitem issue?
|
|
||
| [SerializeField] | ||
| private Transform Anchor; | ||
|
|
There was a problem hiding this comment.
can you verify that you still see this extra line?
|
|
||
| public void OnInputUp(InputEventData eventData) | ||
| { | ||
| } |
There was a problem hiding this comment.
recommend adding a comment to the effect of "this method intentionally left blank"
|
|
||
| namespace HoloToolkit.UX.ToolTips | ||
| { | ||
| public static class ToolTipUtility |
|
|
||
| default: | ||
| // For all other types, just use the array position | ||
| // TODO error checking for array size (?) |
There was a problem hiding this comment.
this TODO sounds like a good idea :)
| TopRightCorner = 6, | ||
| TopLeftCorner = 7, | ||
| // Automatic options | ||
| Center, |
There was a problem hiding this comment.
If ClosestCorner becomes 0, it will be important to also update the dev branch. otherwise apps will get an unexpected setting change when migrating
…Toolkit-Unity into Patch4_Dev_Tooltip
rolled back initial pushed change concerning ClosestCorner = 0 rearrangement of enum values because first 8 enum vals are used as array indices and cannot be rearranged.
…xedRealityToolkit-Unity into Patch4_Dev_Tooltip
| [RequireComponent(typeof(ToolTipConnector))] | ||
|
|
||
| /// <summary> | ||
| /// Class for Tooltip object |
There was a problem hiding this comment.
nit: not really needed. the following line describes the class
| set | ||
| { | ||
| showBackground = value; | ||
| GetComponent<ToolTipBackgroundMesh>().IsVisible = value; |
There was a problem hiding this comment.
what should happen if GetComponent fails? is there a cleaner solution than a nullref exception?
| set | ||
| { | ||
| showOutline = value; | ||
| GameObject TipBackground = contentParent.transform.GetChild(1).gameObject; |
There was a problem hiding this comment.
can the child be at an index other than 1?
| { | ||
| showOutline = value; | ||
| GameObject TipBackground = contentParent.transform.GetChild(1).gameObject; | ||
| Rectangle rectangle = TipBackground.GetComponent<Rectangle>(); |
There was a problem hiding this comment.
what should happen if GetComponent fails?
| { | ||
| showConnector = value; | ||
| //todo fix this | ||
| Line lineScript = GetComponent<Line>(); |
There was a problem hiding this comment.
what if GetComponent returns null?
| toolTipText = value; | ||
| RefreshLocalContent(); | ||
| if (ContentChange != null) | ||
| ContentChange.Invoke(); |
|
|
||
| Transform pivotTransform = transform.Find("Pivot"); | ||
| Transform anchorTransform = transform.Find("Anchor"); | ||
| if (pivotTransform == null || anchorTransform == null) { |
There was a problem hiding this comment.
please align the {'s in these ifs
| return false; | ||
| } | ||
| Transform contentParentTransform = pivotTransform.Find("ContentParent"); | ||
| if (contentParentTransform == null) { |
There was a problem hiding this comment.
please align the {'s in these ifs
| return false; | ||
| } | ||
| Transform labelTransform = contentParentTransform.Find("Label"); | ||
| if (labelTransform == null) { |
There was a problem hiding this comment.
please align the {'s in these ifs
| } | ||
|
|
||
| #if UNITY_EDITOR | ||
| private void OnDrawGizmos() { |
| break; | ||
|
|
||
| default: | ||
| throw new System.ArgumentOutOfRangeException("ScaleMode not set to valid enum value."); |
There was a problem hiding this comment.
is there a way to ensure it cannot?
|
I see some missing prefabs and a lot of newly generated meta files when I open this now. |
Overview
Tooltips were created to provide floating text panels that are connected to a point on a gameobject and give information about the object. they can be made to appear under different conditions. The Tooltip Examples project was originally created in MRDL and was ported to MRTK_Dev branch.
Target Branch
This PR is based on Patch4_Dev branch. Target branch is may18_dev for May release.
Changes
This PR implements changes involving changing MRTK namespace features to HoloToolkit namespace features.