MRDL -> MRTK: Line creation / rendering classes #1305
Conversation
| @@ -0,0 +1,7 @@ | |||
| //--------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
What's up with this .fx asset?
There was a problem hiding this comment.
I'm not sure! It seems like it's getting auto-generated, but I don't know how or why. @keveleigh noticed this in the other pull request as well. Theories, anyone?
There was a problem hiding this comment.
Disable the material import on the mesh asset.
There was a problem hiding this comment.
That's where we're stumped, it's already disabled.
There was a problem hiding this comment.
Could we remove this, and it's meta file until we get it sorted out?
| using System; | ||
| using UnityEngine; | ||
|
|
||
| namespace MRTK.UX |
There was a problem hiding this comment.
We should probably update the namespace to match the toolkits.
There was a problem hiding this comment.
How's MRTK.UX -> Holotoolkit.Unity.UX?
There was a problem hiding this comment.
Fantastic. Also, please be sure to make sure the namespaces in the rest of this folder are also correct.
| public int CompareTo(Distorter other) | ||
| { | ||
| if (other == null) | ||
| return 0; |
| /// <returns></returns> | ||
| protected abstract Vector3 DistortScaleInternal(Vector3 point, float strength); | ||
|
|
||
| protected void OnEnable() |
There was a problem hiding this comment.
should these also be virtual?
There was a problem hiding this comment.
I'll make OnEnable / OnDisable virtual.
DistortPoint/ScaleInternal are abstract to make it clear that new Distorters must implement those functions in order to work.
| public class FastSimplexNoise | ||
| { | ||
| private const double STRETCH_2D = -0.211324865405187; | ||
| private const double STRETCH_3D = -1.0 / 6.0; |
There was a problem hiding this comment.
Why do the math if we know what the values are going to be?
There was a problem hiding this comment.
This tells you that the value should probably be normalized:
const float SOME_NORMALIZED_VALUE = 1f / 25;
This is a little more vague:
const float SOME_MYSTERIOUS_VALUE = 0.04f;
I'll update the other numbers to follow the same 1 / x format, for consistency.
There was a problem hiding this comment.
That's what summary xml comments are for.
| new int[] { 3, 1, 1, 1, 0, 3, 1, 1, 0, 1, 3, 1, 0, 1, 1, 3, 0, 1, 1, 1, 4, 1, 1, 1, 1 }, | ||
| new int[] { 1, 1, 0, 0, 0, 1, 0, 1, 0, 0, 1, 0, 0, 1, 0, 1, 0, 0, 0, 1, 2, 1, 1, 0, 0, 2, 1, 0, 1, 0, 2, 1, 0, 0, 1, 2, 0, 1, 1, 0, 2, 0, 1, 0, 1, 2, 0, 0, 1, 1 }, | ||
| new int[] { 3, 1, 1, 1, 0, 3, 1, 1, 0, 1, 3, 1, 0, 1, 1, 3, 0, 1, 1, 1, 2, 1, 1, 0, 0, 2, 1, 0, 1, 0, 2, 1, 0, 0, 1, 2, 0, 1, 1, 0, 2, 0, 1, 0, 1, 2, 0, 0, 1, 1 } | ||
| }; |
There was a problem hiding this comment.
We can probably cache all these new integer arrays as well instead of creating them each time.
| } | ||
| seed = seed * 6364136223846793005L + 1442695040888963407L; | ||
| seed = seed * 6364136223846793005L + 1442695040888963407L; | ||
| seed = seed * 6364136223846793005L + 1442695040888963407L; |
There was a problem hiding this comment.
Should these values be constant?
| public class Bezeir : LineBase | ||
| { | ||
| [Serializable] | ||
| public struct PointSet |
There was a problem hiding this comment.
If we're referencing this PointSet outside of Bezeir we should move it into it's own class, otherwise it should be private.
There was a problem hiding this comment.
I've made them private. The only other class that needs to know about them is the Bezier editor.
|
|
||
| #if UNITY_EDITOR | ||
| [UnityEditor.CustomEditor(typeof(Bezeir))] | ||
| public class CustomEditor : LineBaseEditor |
There was a problem hiding this comment.
Nit: Editor scripts should be in their own classes.
There was a problem hiding this comment.
I think this one's worth discussing. There are benefits to keeping some editors nested under the scripts they target like this.
Eg the latest Spline class, where 'points' is now private. Other scripts are forced to access these points via GetPoint and SetPoint, which keeps things clean. But the Editor script still has direct access to the 'points' array, allowing it to do some error-checking stuff that wouldn't be possible otherwise.
To get the same functionality with a separated Editor you'd have to define editor-only public properties or some other potentially confusing kludge.
Devs also benefit from seeing that a custom editor has been declared and from seeing the logic inside it without having to go on a scavenger hunt.
| /// <returns></returns> | ||
| protected abstract float GetUnclampedWorldLengthInternal(); | ||
|
|
||
| // Public |
There was a problem hiding this comment.
regions would be better
| Vector3 origin = transform.TransformPoint(OriginOffset); | ||
| rotationVector = (point - origin).normalized; | ||
| break; | ||
|
|
There was a problem hiding this comment.
nit: extra line spacing
| } | ||
|
|
||
| if (rotationVector.magnitude < MinRotationMagnitude) | ||
| return transform.rotation; |
| /// <summary> | ||
| /// Base class for line editors (splines, bezeirs, parabolas, etc) | ||
| /// </summary> | ||
| public class LineBaseEditor : MRTKEditor |
There was a problem hiding this comment.
Editor scripts should live in an Editor folder
There was a problem hiding this comment.
This extends MRTKEditor, which must be kept outside Editor folders in order to function.
There was a problem hiding this comment.
I really really don't like this pattern.
There was a problem hiding this comment.
Yes I can tell it makes your skin crawl, lol. Maybe we should open a dedicated issue to discuss the pros / cons.
There was a problem hiding this comment.
your skin crawl
🤣 I wouldn't go that far, but yes. It goes against all the industry standard best practices.
There was a problem hiding this comment.
It goes against all the industry standard best practices.
Unity-land is a weird place, man. Sometimes down is up and black is white. I long for the clarity of a pure C# repo.
There was a problem hiding this comment.
Haha, Unity development is a different beast. I think a lot of traditional software engineers get tripped up with some things.
|
|
||
| int previewResolution = Mathf.Min(linePreviewResolutionSelected, linePreviewResolutionUnselected); | ||
| if (selected) | ||
| previewResolution = linePreviewResolutionSelected; |
| if (wrap) | ||
| return WrapIndex(index, Objects.Count); | ||
| else | ||
| return Mathf.Clamp(index, 0, Objects.Count - 1); |
There was a problem hiding this comment.
was there an update for this?
There was a problem hiding this comment.
It's been committed.
| [Range(0f, 1f)] | ||
| public float NormalizedDistance = 0f; | ||
|
|
||
| private void Update() { |
| public Vector3 AxisSpeed = Vector3.one; | ||
| public Vector3 AxisOffset = Vector3.zero; | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: extra line spacing
| private Vector3[] prevPoints; | ||
| private System.Random randomPosition; | ||
| private System.Random randomRotation; | ||
| private FastSimplexNoise noise = new FastSimplexNoise(); |
There was a problem hiding this comment.
nit: fields should be declared at the top of the class
There was a problem hiding this comment.
Not seeing this change yet.
There was a problem hiding this comment.
This has been committed as well.
| public class LineParticles : LineRendererBase | ||
| { | ||
| const int GlobalMaxParticles = 2048; | ||
| const float GlobalParticleStartLifetime = 0.5f; |
There was a problem hiding this comment.
private or public accessors?
There was a problem hiding this comment.
I don't think they're needed since they're const & can't be modified.
There was a problem hiding this comment.
I think it's always good to know, that way future devs know what scope they're in.
There was a problem hiding this comment.
I'm a little confused because a const can't use get / set accessors. Are you thinking we should change these to public properties that return constant values?
There was a problem hiding this comment.
It's just helpful to know if this information should have the ability to be accessed outside the scope of just this class. Idk it's really not a big deal here. Just my 2c
There was a problem hiding this comment.
No worries, I just use const all the time so I wanted to get on the same page. If you post an 'ideal scenario' I'll modify the class.
| main.playOnAwake = false; | ||
| main.maxParticles = Mathf.Min (MaxParticles, GlobalMaxParticles); | ||
| main.simulationSpace = ParticleSystemSimulationSpace.World; | ||
| //main.useUnscaledTime = true; |
| public class LineUnity : LineRendererBase | ||
| { | ||
| const string DefaultLineShader = "Particles/Alpha Blended"; | ||
| const string DefaultLineShaderColor = "_TintColor"; |
There was a problem hiding this comment.
missing access modifiers
There was a problem hiding this comment.
Not needed since they're const.
I could make them static with a Get accessor? eg:
public static string DefaultLineShader { get { return "Particles / Alpha Blended"; } }
There was a problem hiding this comment.
So what happens if someone tries to use a custom shader for line rendering?
There was a problem hiding this comment.
These are just failsafe values. If they specify a material and property name, that material's shader is used. If they leave the material null, a material is generated at runtime using the "Particles/Alpha Blended" shader.
| { | ||
| #region structs | ||
| [Serializable] | ||
| public struct SplinePoint |
There was a problem hiding this comment.
nit: put in it's own file
|
|
||
| public static readonly Vector3 DefaultUpVector = Vector3.up; | ||
|
|
||
| public enum InterpolationEnum |
There was a problem hiding this comment.
Each of the enums in this class could be in their own files as well, and probably don't need to be in this LineUtils class
There was a problem hiding this comment.
Moved all enums out of the LineUtils class.
We should still keep them in a single file, either LineUtility.cs, or possible in LineBase.cs alongside the LineBase class. Or maybe we could compromise and put them in a single enum-only class called LineBaseEnums.cs.
Here's my reasoning - clumping enums together with the class that interprets them is informative. It's a form of documentation that says: these things all related - taken together, they form a complete picture of this object's possible behavior.
Breaking them up into separate files destroys that information while adding complexity. Devs have to work backwards from variables one-by-one, forensically reconstructing relationships that should be clear at a moment's glance.
| protected virtual void OnEnable() | ||
| { | ||
| if (source == null) | ||
| source = gameObject.GetComponent<LineBase>(); |
There was a problem hiding this comment.
I've noticed we get the line base line a lot. Maybe we should also require the component on the classes that use it.
There was a problem hiding this comment.
LineBase is abstract so we can't require it. But this actually is why we created the [UseWithAttribute] - I'll add those attributes now.
| return; | ||
|
|
||
| if (source == null) | ||
| source = gameObject.GetComponent<LineBase>(); |
There was a problem hiding this comment.
you could even make source a property and get the component if it hasn't yet been initialized.
Also: braces
|
|
||
| MeshFilter stripMeshFilter = stripMeshRenderer.GetComponent<MeshFilter>(); | ||
| if (stripMeshFilter == null) | ||
| stripMeshFilter = stripMeshRenderer.gameObject.AddComponent<MeshFilter>(); |
There was a problem hiding this comment.
we could use gameObject.EnsureComponent<MeshFilter>() here too.
Also : braces
There was a problem hiding this comment.
Good idea. Really useful extension.
| private void OnDisable() | ||
| { | ||
| if (lineMatInstance != null) | ||
| GameObject.Destroy(lineMatInstance); |
There was a problem hiding this comment.
nit braces.
Also, are we handling the case if we're rendering the line when we're not in playmode? (or does this not get rendered if we're not playing?)
There was a problem hiding this comment.
It only renders the mesh in play mode. Otherwise it uses gizmos.
|
|
||
| public static void GenerateStripMesh(List<Vector3> positionList, List<Color> colorList, List<float> thicknessList, float uvOffsetLocal, List<Vector3> forwardList, Mesh mesh) | ||
| { | ||
| // TODO store these as local variables to reduce allocations |
There was a problem hiding this comment.
Let's go ahead and do this TODO. seems simple enough
| if (lineRenderer == null) | ||
| lineRenderer = gameObject.GetComponent<UnityEngine.LineRenderer>(); | ||
| if (lineRenderer == null) | ||
| lineRenderer = gameObject.AddComponent<UnityEngine.LineRenderer>(); |
There was a problem hiding this comment.
ensure component here too
| { | ||
| lineRenderer.enabled = true; | ||
|
|
||
| switch (this.StepMode) |
There was a problem hiding this comment.
why call this.StepMode are we hiding an inherited property?
There was a problem hiding this comment.
Legacy cruft. Removed.
| [SerializeField] | ||
| private UnityEngine.LineRenderer lineRenderer; | ||
|
|
||
| private Vector3[] positions; |
There was a problem hiding this comment.
fields should be declared at the top of the class
|
|
||
| public static class LineUtils | ||
| { | ||
| #region enums |
There was a problem hiding this comment.
we can probably remove this region now
| public static Vector3 GetVectorCollectionBlend(Vector3[] vectorCollection, float normalizedLength, bool repeat) | ||
| { | ||
| if (vectorCollection.Length == 0) | ||
| return Vector3.zero; |
There was a problem hiding this comment.
more braces ;) Here and down the rest of this class
| protected override Vector3 GetPointInternal(float normalizedDistance) | ||
| { | ||
| if (points == null || points.Length != 8) | ||
| points = new Vector3[8]; |
There was a problem hiding this comment.
braces here and below
| return; | ||
|
|
||
| // Only draw a gizmo if we don't have a line renderer | ||
| LineRendererBase lr = gameObject.GetComponent<LineRendererBase>(); |
There was a problem hiding this comment.
we should cache this instead of getting the component each update.
There was a problem hiding this comment.
Normally I'd agree but I don't want to add a new field to the class for the sake of a Gizmos function. Given that Lines can have multiple LineRenderers I worry that its presence would be confusing for devs.
Since we only do the GetComponent call while the editor isn't playing it won't be a performance issue.
| return points[NumPoints - 1].Point; | ||
| } | ||
| if (point1Index < 0) | ||
| return points[0].Point; |
| protected override Vector3 GetPointInternal(int pointIndex) | ||
| { | ||
| if (pointIndex < 0 || pointIndex >= points.Length) | ||
| throw new IndexOutOfRangeException(); |
| float arrayValueLength = 1f / points.Length; | ||
| int indexA = Mathf.FloorToInt(normalizedLength * points.Length); | ||
| if (indexA >= points.Length) | ||
| indexA = 0; |
| // Convenience buttons for adding / removing points | ||
| protected override void DrawCustomFooter() | ||
| { | ||
|
|
|
|
||
| Spline line = (Spline)target; | ||
|
|
||
| HashSet<int> overlappingPointIndexes = new HashSet<int>(); |
There was a problem hiding this comment.
Do we want to create a new hash set each time we draw the footer?
Maybe we can cache this.
There was a problem hiding this comment.
Cached as a static variable.
| { | ||
| if (GUILayout.Button(" + Add Points to Start")) | ||
| { | ||
| List<SplinePoint> points = new List<SplinePoint>(); |
There was a problem hiding this comment.
We can probably cache this list and clear it each time we add a point
There was a problem hiding this comment.
Cached as a static variable.
| for (int j = 0; j < line.points.Length; j++) | ||
| { | ||
| if (j == i) | ||
| continue; |
| else if (drawOverrideAttributes.Length > 0) | ||
| { | ||
| if (drawOverrideAttributes.Length > 1) | ||
| DrawWarning("You should only use one DrawOverride attribute per member. Drawing " + drawOverrideAttributes[0].GetType().Name + " only."); |
|
|
||
| Handles.color = handleColorAxis; | ||
| if (autoSize) | ||
| handleSize = Mathf.Lerp(handleSize, HandleUtility.GetHandleSize(position) * handleSize, 0.75f); |
There was a problem hiding this comment.
braces here and below
|
Holy guacamole that was a lot of style changes. Everything has been committed. |
This pull request includes classes for line creation, manipulation & rendering, including:
Lines
Renderers
These classes are required in the upcoming Navigation & Pointers Academy tutorial. Since time is a factor I'm creating a separate pull request to ensure they aren't held up by revisions of unrelated Stage 2/3 MRDL elements.
A test scene has been added to Holotoolkit-Examples/UX/Scenes/LineExamples.
@cre8ivepark we could use your input on this scene as it's a bit ugly.
Looking forward to your feedback /suggestions,
-L