Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vNEXT - UX Lines feature port #2513

Conversation

StephenHodgson
Copy link
Contributor

@StephenHodgson StephenHodgson commented Jul 29, 2018

Overview

UX Lines feature port (required for pointer ray visualization)

image

The Mixed Reality Toolkit's built in lines are split into two components: Data and Renderers.

Line Data Providers

Line components that define all the characteristics of that line and all the data associated with it. This does not handle any aspect of rendering a line in the scene, and can be used for paths, interpolation, and more.

  • Simple Line – a line with two points
  • Parabola – parabolic arc between two points
  • Rectangle – a line that loops back on itself with 4 corners
  • Ellipse – a line that loops back on itself with no corners
  • Spline – a multi-point line with Bezier interpolation

Line Renderers

The components take the line data and render it in the scene.

  • MixedRealityLineRenderer – implements Unity's built in line renderer component, and applies the line data to it
  • ParticleSystemLineRenderer – attaches a set of particles to the line
  • MeshLineRenderer – creates insances of a mesh along the line
  • StripMeshLineRenderer – draws a strip of polygons along the line

Other changes outside of line additions

Some things to discuss

  • I'm using Text Mesh Pro assets in the example scene, and think they look better way better. I'd like to see us use the SDF text for all the example scenes.
  • Do we want to have the lines in the core?
    • I initially put them there, so when people get the core, they don't have to get the SDK if they want teleportation and pointer lines out of the box.
  • Is there anything else that we want to add to the demo scene?

Known Issues

@StephenHodgson
Copy link
Contributor Author

If you guys have any other summary or documentation changes, I'm happy to accept PRs

@SimonDarksideJ
Copy link
Contributor

Document ALL the things :P

@@ -0,0 +1,8 @@
fileFormatVersion: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Text Mesh Pro being checked in as an option (and all our components will also work with Unity text) or as the way the Toolkit does text? Seems like something not everybody needs, and would add dependencies to anybody who wants to use our UI/UX text components when Unity text would suffice. Not a huge fan of that being added.

Copy link
Contributor Author

@StephenHodgson StephenHodgson Jul 30, 2018

Choose a reason for hiding this comment

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

I very much disagree. The quality different in TMP vs default unity text in VR/AR is quite astonishing. Using SDF should be the industry standard.

Seeing as TMP is supposed to replace the old text renderers, (It's been depreciated) and TMP is a default asset with Unity now, and its ease to put into projects via the package manager makes it ideal.

I'm not suggesting we put it in the SDK or Core components, but we should def be using it for our demo, tutorial, and example scenes.

Choose a reason for hiding this comment

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

We should definitely not be including it in the SDK / Core (we want those as light as possible).

I can see the value of using it in the examples.

@StephenHodgson, how is it being included? Are we checking it into the examples tree? If so, does their license allow that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's owned by Unity. Not sure if a license is required if you're using the editor to begin with.

Copy link
Contributor Author

@StephenHodgson StephenHodgson Jul 30, 2018

Choose a reason for hiding this comment

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

Are we checking it into the examples tree?

As stated in #2513 (comment), I put them in the default download location that the package manager prompts you to place them.

Choose a reason for hiding this comment

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

I put them in the default download location that the package manager prompts you to place them.

Sorry, not sure if I am reading this right... does this mean the files are checked in? Or do they get automatically added somehow.

It's owned by Unity. Not sure if a license is required if you're using the editor to begin with.

I am concerned with the license to redist the package.

Copy link
Contributor Author

@StephenHodgson StephenHodgson Jul 30, 2018

Choose a reason for hiding this comment

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

A relevant forum thread.

At some point the old Text component and Text Mesh will be deprecated but that will come after the new text system / components are available.

Here's another.

As per the title and assuming all goes well, a new version of TextMesh Pro will be included by default in Unity 2018.1.

Later on in the thread it's confirmed that 2018.2 has the TMP assets by default.

With Unity 2018.2, the TextMesh Pro UPM package is included by default. However, the TMP Essential Resources are not imported / added to a user project until first use and consent of the user. Alternatively, a user can manually import the TMP Essential Resources via the "Window - TextMeshPro - Import TMP Essential Resources" menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am concerned with the license to redist the package.

This is a good point. I suppose it doesn't hurt to remove the TMP assets from the project itself and let users download on their own when prompted. Instead I'll just make the folder ignored in git. Sound good?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Reference and even prompt people to download it, but don't actually include it.
In 2018.2, we can solve this better by adding it as a dependant package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 2018.2, we can solve this better by adding it as a dependent package

It's already in the editor in 2018.2, and installed. So no worries there.

@keveleigh keveleigh mentioned this pull request Jul 30, 2018
@SimonDarksideJ
Copy link
Contributor

SimonDarksideJ commented Jul 30, 2018

Can you update the description to list any remaining features @StephenHodgson

Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

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

Almost there, just some more docs and some math functions required

@@ -0,0 +1,173 @@
%YAML 1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, materials / textures for the SDK should be in the standard assets folder

namespace Microsoft.MixedReality.Toolkit.Inspectors.Utilities.Lines.DataProviders
{
[CustomEditor(typeof(EllipseLineDataProvider))]
public class EllipseLineDataProviderInspector : BaseMixedRealityLineDataProviderInspector
Copy link
Contributor

Choose a reason for hiding this comment

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

Nudge

namespace Microsoft.MixedReality.Toolkit.Inspectors.Utilities.Lines.DataProviders
{
[CustomEditor(typeof(ParabolaPhysicalLineDataProvider))]
public class ParabolaPhysicalLineDataProviderInspector : BaseMixedRealityLineDataProviderInspector
Copy link
Contributor

Choose a reason for hiding this comment

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

Except for vNext, they should.

get { return radius; }
set
{
if (value.x < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, should use standard math functions for min and max values


protected void OnEnable()
{
if (lineMaterial == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's just that we have "if null / Debug" repeated everywhere.


namespace Microsoft.MixedReality.Toolkit.Internal.Utilities.Physics.Distorters
{
public abstract class Distorter : MonoBehaviour, IComparable<Distorter>
Copy link
Contributor

Choose a reason for hiding this comment

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

Summary


namespace Microsoft.MixedReality.Toolkit.Internal.Utilities.Physics.Distorters
{
public class DistorterBulge : Distorter
Copy link
Contributor

Choose a reason for hiding this comment

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

Summary

SimonDarksideJ
SimonDarksideJ previously approved these changes Jul 31, 2018
@david-c-kline david-c-kline merged commit c05ad6f into microsoft:mrtk_development Jul 31, 2018
@@ -4,7 +4,7 @@
QualitySettings:
m_ObjectHideFlags: 0
serializedVersion: 5
m_CurrentQuality: 5
m_CurrentQuality: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cause I was testing on HoloLens it might have changed the quality to the lowest.

@StephenHodgson StephenHodgson deleted the vNEXT-PointerRayVisualization branch July 31, 2018 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants