Search Unity

  1. Welcome to the Unity Forums! Please take the time to read our Code of Conduct to familiarize yourself with the forum rules and how to post constructively.

Feedback Splines Feedback

Discussion in 'World Building' started by Rowlan, Oct 30, 2021.

  1. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    I'm assuming that storing it three ways is because the conversion is expensive? In any case, from a non-programer perspective storing in any format that massively changes the position of that data when knots are changed feels broken. And while there are valid reasons to want the data in any of these formats, it seems like it should only operate to the user as data which does not exhibit bad behavior. For my system, I feed it to my shaders in knot space because I can use the T value from a bezier interpolation to quickly lookup the associated data - but then this creates an issue for users if I store it that way. So I suppose what I'll have to do is store it in distance, then convert it into knot space for my shaders. I guess the valid use case for knot space is if people want to associate data directly with the knots, and not allow users to move it off-knot, etc. Hmm...

    Which is implemented in exactly none of the examples. Now, one can argue that examples don't have to be production ready, but I think when coming from Unity they should at least be a good guide into how to make something production ready, because it's a sure fire way for you to see all the issues that come up, and the amount of code which would need to be written to make things compatible with your system, as well as test that system. And likely you'd come up with an abstraction which handles this all automatically (or with a lot less code) after writing it several times. And since this code will be copied by many people, things that code does not handle will propagate to every system made with it.

    I think being able to reference a spline in serialization would be a big help. Having to do all this by index and keep it all in sync via callbacks is a fragile affair, and easy to get wrong. Further, tools will be written that do things to spline data outside of either of our code bases, which can easily get that data out of sync in new and interesting ways. So the more direct the binding of the data, the less chance of that happening. Right now, learning this system and handling all its edge cases is about as complex as grabbing an existing open source spline system and modifying it to your needs.

    For instance, the callbacks you mention don't seem to be on Spline Container or the spline class - but assuming I just can't find them, a spline is added or removed from the container - in theory I can reorder the spline data to match but what about when it's reordered in the list? Does that get called as a remove and then an add? Do I have to keep the old data around in case an add comes in which matches it?


    > 3. You have to completely rewrite the spline editing classes to work with multiple splines, as the ones included in the package don't work.

    If something is not working, please let us know with a bug report![/QUOTE]

    Bug reports:

    IN-20272
    IN-20273
    IN-20275

    For the editor, I modified your editor as described a few posts up (to call the editor functions on all the splines instead of just the first one), but only the last spline works. So to fix it I had to pull out all those editor functions and rewrite them (but I've deleted that at this point since there were so many other issues trying to get multi-spline to work).
     
    NotaNaN likes this.
  2. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    Oh, and BTW, this is just my use case of wanted to have spline width, which is now my own class since yours is in samples and doesn't support any of this stuff. Which means any other asset which wants to use width will not recognize my version, and have to add its own concept of width to the spline, such that there are now 2 different width controls.
     
  3. gabrielw_unity

    gabrielw_unity

    Unity Technologies

    Joined:
    Feb 19, 2018
    Posts:
    945
    Apparently this caused some confusion, I'll try to clarify that. Splines isn't abandoned, our team isn't pushed out - we're simply finishing (as of 2.1, still to come) the initial set of features we planned out via UX research. Greyboxing tools are the new feature focus, while Splines matures.

    If you missed the opportunity to connect with the research, genuinely sorry to hear - ironically, it's always a struggle for us to get enough participants early on. Would love to get more early input. I'd say ... try to watch for official posts requesting feedback here on the forum. Or, ping me directly if you see a WIP feature that is really important to you!
     
    frarf, KarimZoPr0, karl_jones and 2 others like this.
  4. AcidArrow

    AcidArrow

    Joined:
    May 20, 2010
    Posts:
    9,784
    No confusion. The feature is "supported".

    The same way, say, probuilder is supported.

    Remind me, can you delete an edge in probuilder yet?

    In any case, I'll go back to supporting my forum presence, so don't bother replying (or a mod can delete this or whatever).
     
  5. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    Yeah, reordering splines will simply break the association, because it doesn't call add or remove events, it just reorders them so the arrays no longer match up.
     
  6. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    Assuming the issues with associating spline data and multi splining get addressed, I'm going to lay out the likely scenario which I think will form with asset store authors using the spline tools (which, IMO, I think is something everyone would benefit from if done right).

    MicroVerse uses Unity splines to offer non-destructive path and area editing to terrains. These can be used for more than paths, you can add the loft mesh component from the examples to easily create a river or road for instance. You can use the SplineInstantiate component to instantiate objects along the side of the path, like fences, etc.

    Rivers need width controls though. Since the width system is in examples, I can either:

    A) Require the user to install the samples and use its width system, which is odd.
    B) Copy the code into my own project, or create my own width system.

    If we follow choice A, then other assets who choose choice A can work with my asset! Yay! But requiring samples to be installed for my asset to compile seems very, very wrong. Plus this installs into the asset folder under a samples directory that screams delete me.

    If we follow choice B, then my asset no longer works with, say, Loft Road, or any other asset which has the concept of width, because that component is either looking for the width system in samples, or for its own custom width implementation. For instance, an AI asset that drives cars within the road area, another river system, etc. So now everyone will be duplicating this code and integrating a variant of it into their asset, so you'll have 4 Loft Road components to choose from which are all slightly different, and potentially getting out of sync with any fixes Unity makes.

    So what you'll end up with is a bunch of partially compatible splines in the scene, with common data that could be shared, but won't because the concept of width is not part of the spline system, but rather custom data. And a bunch of duplicated and modified code, which may or may not be updated. And the solution to this will be similar to @Rowlan 's integration of RAM into MicroVerse - a component that simply slaves the data of one spline system to another, keeping them in sync, or doing all that manually.

    Neither of these choices seem great to me, and I'd argue that while width is a common enough property to exist as part of the main package - I also totally get why you wouldn't want it to be, and would want a system for truly custom data to exist along a spline. Moving width to the main package kicks the can down the road a bit, because it's such a common thing, but there's no reason there wouldn't be other properties that multiple packages find in common (friction for car games, etc).

    So spitballing, I could imagine a system where there can be commonly named/type'd custom data added to splines, which is less general than the current system, but much easier to work with and be shared between projects. For instance, if you could declare a SplineData under a key name and type, something like:

    Code (CSharp):
    1.  
    2.  
    3. // add a new tool to the spline editing options under the key name Width which stores a single float value
    4. // use horizontal handles to adjust the width value
    5. // store that data as normalized 0-1 values
    6. // If this tool already exists, this will be ignored.
    7. SplineUtility.AddSplineDataTool<float>("Width", SplineEditorControl.HorizontalHandles, PathIndexUnit.Normalized);
    Then to get that data:

    Code (CSharp):
    1.  
    2. // note that type and name must match. If someone creates a float2 width, they're creating a different control.
    3. var data = SplineContainer.Spline.GetSplineData<float>("Width");
    4.  
    Then all that's needed to have width be a common thing is to declare the same tool name/type and grab the same data from the spline when reading it. This could just use a fixed up version of the current system to implement this with a few control options and types (types : color, float, vectors controls: horizontal, vertical, input field).

    Now there's a lot I don't love about this, in that it feels kinda undefined, much like having a vector4 on particles for custom data, or some set number of unnamed vertex attributes (texcoord0, texcoord1, etc). But it's practical, and makes adding custom one off data to a spline very easy, while also making it so common conventions for common data types are easy to share between different tools. And it is more general than simply having a Vector4 for user data on splines, or something like that.

    Anyway, I'm certainly not married to this idea - but I think thinking a bit about how Splines, and the common data which tools will require of them, can be easy shared, has a lot of benefits.
     
    Last edited: Oct 19, 2022
    NotaNaN likes this.
  7. kaarrrllll

    kaarrrllll

    Unity Technologies

    Joined:
    Aug 24, 2017
    Posts:
    522
    Performance was not a consideration, as the format is only specifying how a DataPoint.Index is interpreted at the SplineData level. Functionally all three are roughly equivalent in terms of efficiency. The decision to present these options was for flexibility reasons.

    The PathIndexUnit.Knot format is actually not just a knot index, but rather knot index with an interpolation to the next knot stored in the fractional part.

    That's true, we did not put a strong focus on samples for tool developers. The primary focus of our recent work has been usability of Spline tools for users, not the editor API. There is absolutely room for improvement in the editor API, and I expect we will expand this support as we gather feedback.

    Agreed. It's something we have spent a lot of time thinking about as well. It's part of the reason that our editor API is so limited at the moment - we don't have a simple answer for external tool developers yet. What we do internally is fragile and difficult to implement correctly. It's not the API we want to support for our users.

    Yes, these callbacks are invoked from the SplineReorderableListUtility. They may be new as of 2.0 or 2.1, I don't remember off the top of my head.

    The Samples were overhauled for multiple paths somewhat late in the 2.0 previews. Looking at them now I don't see anything obvious not working with multiple Splines, but I may be missing something.

    Thanks, I'll follow up on those. It doesn't look like they've cleared incoming QA yet, I'll see about manually triaging.

    By editor are you referring to the Inspector? I don't understand the problem. If you have a gist or small snippet I could provide a better answer.

    Interestingly, we considered this exact approach when starting work on Splines. I don't remember off-hand why we didn't go with this. If I were to guess, we were likely considering DOTS compatibility and wanting to minimize the footprint of the default types.

    As an aside, thank you for taking the time to raise your feedback. It's clear that you have spent time working with this code and that kind of insight is not something that we take for granted.

    We will be focused on delivering value across multiple features but will also continue our support and maintenance for Splines. Please continue to log bug reports as this is the best method for us to prioritize these efforts.
     
    KarimZoPr0 and NotaNaN like this.
  8. Rowlan

    Rowlan

    Joined:
    Aug 4, 2016
    Posts:
    3,268
    The split by 3 dimensions for the PathIndexUnit is way too complicated, eg having the the range of t different by enum. Personally I'd prefer simple wrapper methods to this approach.

    Suggestion: Take a look at MicroVerse and the Spline usage. It's really super impressive what Jason pulled off and in that regard an excellent advertising for your work on Splines. Really looking forward to more features :)
     
    KarimZoPr0 and Deleted User like this.
  9. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    This is quite a scattering of callbacks all over the code required to simply keep some data in sync.


    Thats the thing about code- until you try to implement something with it, you don't really know if it works. And it's pretty clear no one tried using SplineData<T> with multiple splines.


    I'm referring to the code I posted in this very thread:


     
    KarimZoPr0 likes this.
  10. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    Of note, I'm not saying you have to expand the default data structure here - you could still store it as a bunch of parallel arrays of data, with a dozen callbacks you have to manage from different systems to keep it in sync, but that becomes your problem instead of a problem every engineer using your system has to solve.
     
    KarimZoPr0 likes this.
  11. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    Also, SplineReorderableListUtility is private, so we can't use it to register with the reorderable callbacks as you recommend..
     
    Last edited: Nov 11, 2022
    KarimZoPr0 likes this.
  12. Ereroa

    Ereroa

    Joined:
    Apr 27, 2017
    Posts:
    8
    How to I get to add a knot in spline.
    I can't get both Insert and Add to work.

    I want to add positions of all the child gameobject in one parent gameobject as a knots in spline.

    Code (CSharp):
    1. using System.Collections;
    2. using System.Collections.Generic;
    3. using UnityEngine;
    4. using UnityEngine.Splines;
    5.  
    6. public class SplineGrabCurvy : MonoBehaviour
    7. {
    8.  
    9.     public GameObject CurvySpline;
    10.     public List<Vector3> positions = new List<Vector3>();
    11.     private Spline spline;
    12.  
    13. void Awake()
    14. {
    15.     Spline spline = GetComponent<Spline>();
    16. }
    17.  
    18.    public void GrabChildPositions()
    19.    {
    20.     Debug.Log("Say Something");
    21.     positions.Clear();
    22.     int pathChild = CurvySpline.transform.childCount;
    23.             for (int i = 0; i < pathChild; i++)
    24.         {
    25.             positions.Add(new Vector3(CurvySpline.transform.GetChild(i).transform.localPosition.x, CurvySpline.transform.GetChild(i).transform.localPosition.y, CurvySpline.transform.GetChild(i).transform.localPosition.z));
    26.         }
    27.    }
    28.     /// <param name="spline"></param>
    29.       public void GenerateKnots()
    30.    {
    31.  
    32.  
    33.         //spline.clear();
    34.         int pathChild = CurvySpline.transform.childCount;
    35.              
    36.         for (int i = 0; i < pathChild; i++)
    37.             {
    38.                 BezierKnot pKnot = new BezierKnot(CurvySpline.transform.GetChild(i).transform.localPosition,0,0,Quaternion.identity);
    39.                 //float3 point = new float3(CurvySpline.transform.GetChild(i).transform.localPosition.x, CurvySpline.transform.GetChild(i).transform.localPosition.y, CurvySpline.transform.GetChild(i).transform.localPosition.z);
    40.                 //transform.GetComponent<Spline>().Spline.Add(pKnot, TangentMode.AutoSmooth);
    41.                 spline.Insert(i, pKnot, TangentMode.AutoSmooth);
    42.  
    43.                 //spline.Add(pKnot);
    44.                 //spline[i] = knot;
    45.             }
    46.          
    47.    }
    48. }
     
  13. kaarrrllll

    kaarrrllll

    Unity Technologies

    Joined:
    Aug 24, 2017
    Posts:
    522
    Try this
    Code (CSharp):
    1. class CreateSplineFromChildren : MonoBehaviour
    2. {
    3.     Spline m_Spline;
    4.  
    5.     void Awake()
    6.     {
    7.         m_Spline = GetComponent<SplineContainer>().Spline;
    8.     }
    9.  
    10.     void Update()
    11.     {
    12.         var childCount = transform.childCount;
    13.         var knots = new List<BezierKnot>(childCount);
    14.         for (int i = 0, c = childCount; i < c; ++i)
    15.             knots.Add(new BezierKnot(transform.GetChild(i).position));
    16.         m_Spline.Knots = knots;
    17.         m_Spline.SetTangentMode(new SplineRange(0, childCount), TangentMode.AutoSmooth);
    18.     }
    19. }
    20.  
     
  14. Rowlan

    Rowlan

    Joined:
    Aug 4, 2016
    Posts:
    3,268
    Are there plans about making the selection API public? If not, it would otherwise be helpful to have a helper function that gets the current selection of splines and knots. And an event handler for selection changes. Just checked 2.1, there's no way for me to get the selection via API.
     
  15. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    Spline width not being in the main package means I have to copy all the code which uses width out of the samples and modify it to use my width component instead. This is basically just encouraging everyone to branch their own versions of your components and let them get out of date, etc. It also means when someone adds, say, loft road to their spline, they end up with two different sets of width control points on their spline which is uber confusing and means they have to match the second set of widths to the first set.
     
    shikhrr likes this.
  16. DuncanFewkes

    DuncanFewkes

    Joined:
    May 2, 2017
    Posts:
    2
    Yeah, I came across this too. Seems like a very simple use case - have thing following the spline with ability to change the thing's speed. But it just jumps around like mad because when you change maxSpeed, it calls CalculateDuration() and sets a new m_Duration for the entire spline length, then jumps the follower to position based on current elapsed time and the new duration for the entire spline.

    I mean, c'mon!
    Why am I having to override "SplineAnimate" to hack out huge parts of it just to get something to follow a spline at a dynamic speed?
     
  17. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,360
    I think partially because it's in examples, and the current split between what people expect from a spline system and what's in examples and whats in the package is just a bit off. Most spline packages on the store would have the examples as reusable and well formed functions of the spline system, not examples that may not do what you want and are expected to be modified. Spline width is also another one that is strangely in examples - like it's great to show off how to do custom data with the spline package, but I really don't want 3 different width controls on my spline because 3 different systems needed it and added their own version because they can't use the one in samples reliably.
     
    DuncanFewkes likes this.
  18. DuncanFewkes

    DuncanFewkes

    Joined:
    May 2, 2017
    Posts:
    2
    I suppose so. It just hits me as bizarre that they've got all these examples laid out, some of which do an awful lot of cool stuff, but then the very basic "make thing follow, but allow me to change speed at runtime" is absent.
    Even the Car follow example that I found after posting that rant doesn't have it! I thought I'd have to come back and apologise because I'd been daft and missed that the functionality was staring me in the face, but nope - in the car example I can add different speeds to a list of knots in the spline (or somesuch?) - but not change the car's speed at runtime by pressing buttons or from a different script (my use-case is changing speed according to audio level of whatever music is playing on the PC).

    It wasn't a lot of effort to hack one together, just surprising that it wasn't already there. Ah well.
     
  19. adslitw

    adslitw

    Joined:
    Aug 23, 2012
    Posts:
    248
    @kaarrrllll @gabrielw_unity how would you recommend I increase the thickness of the 'gizmo line' (i.e. the thin line drawn in the editor for each spline)? It's useful to use it as a visualisation to see where splines are missing in levels, but it's so thin it's very hard to see! I've tried changing the colour, but some kind of thickness control would be awesome too.
     
  20. jonagill_

    jonagill_

    Joined:
    Oct 5, 2019
    Posts:
    2
    It looks like `SplineComponent.AlignAxis` is basically busted in Inspectors in Splines 2.1.0, as there is an extremely hacky property drawer that forces it to use `SplineInstantiateEditor`-specific behavior everywhere it's rendered. This causes random enum values to be unselectable in the inspectors for my custom components. Can this PropertyDrawer be removed or rewritten so that we can serialize AlignAxes like any regular enum?
     

    Attached Files:

  21. gabrielw_unity

    gabrielw_unity

    Unity Technologies

    Joined:
    Feb 19, 2018
    Posts:
    945
    Hey! :) Hrm, can you file this as a bug? Seems pretty clear to me that it shouldn't work this way. Sorry for the trouble!

    Good point, I think we're all aware of this but it keeps falling off the stack. I'll check :)