Conversation
|
|
||
| namespace Chisel.Core | ||
| { | ||
| public class Test : MonoBehaviour |
There was a problem hiding this comment.
This should, of course, not be part of a non draft PR. But you already know that
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Nitpicking here, but when you do make a PR, try not to include files where the only change is whitespace. It just makes going through the history of a file more confusing
There was a problem hiding this comment.
Will clean, thanks for the catch
| public class PropertyNode<T> : Node, IPropertyNode where T : GraphProperty | ||
| { | ||
| [HideInInspector] | ||
| public T property; |
There was a problem hiding this comment.
shouldn't this be private?
| public abstract class GraphProperty | ||
| { | ||
| public string Name; | ||
| public bool overrideValue; |
There was a problem hiding this comment.
nitpicking, but override should start with upper case letter, just like Name
There was a problem hiding this comment.
actually, makes more sense to make Name lowercase since they're both fields
| [CreateAssetMenu(fileName = "New Chisel Graph", menuName = "Chisel Graph")] | ||
| public class ChiselGraph : NodeGraph | ||
| { | ||
| public ChiselGraphNode active; |
There was a problem hiding this comment.
since the active node is set through SetActiveNode, wouldn't it make sense to make this either readonly ( get; private set; } or just private?
There was a problem hiding this comment.
The user selected active node is basically the root tree node, the graph parse starts there, so it need to be serialized
Public is definitely not right form a API standpoint I guess, maybe [SerializeField]private is better
There was a problem hiding this comment.
[SerializeField] private would be better imho yeah, since I can imagine a user not understanding the code and try to modify the active node for whatever reason, and then being confused things going crazy
| public ChiselGraphNode active; | ||
| public ChiselGraphInstance instance; | ||
|
|
||
| public List<GraphProperty> properties; |
| public class ChiselGraph : NodeGraph | ||
| { | ||
| public ChiselGraphNode active; | ||
| public ChiselGraphInstance instance; |
There was a problem hiding this comment.
But it seems that a graph is tied 1:1 to an instance? I guess this is a temporary thing to get started?
There was a problem hiding this comment.
Can have multiple instance using the same graph, but you are right, temporary
|
|
||
| for (int i = 0; i < graph.properties.Count; i++) | ||
| if (graph.properties[i].Name != properties[i].Name) | ||
| InitProperties(); |
There was a problem hiding this comment.
OnValidate is called a lot in Unity, and string comparisons are slow. This loop will get slower and slower the more complicated the graph is, so it makes sense to find another way of doing this that scales better
There was a problem hiding this comment.
Will hashset be better? btw anyway to sync the properties without destroying existing properties value?
There was a problem hiding this comment.
Is it possible to set a flag when a name changes instead? Or at least limit this to when editing the graph itself somehow?
| public Dictionary<string, GraphProperty> overriddenProperties; | ||
|
|
||
| CSGTree tree; | ||
| List<Mesh> meshes; |
There was a problem hiding this comment.
can be
readonly List meshes = new List()
and you can remove the "meshes = new List();" in Start below
There was a problem hiding this comment.
same goes for properties/overriddenProperties
|
|
||
| void Update() | ||
| { | ||
| UpdateCSG(); |
There was a problem hiding this comment.
I assume UpdateCSG is here just for testing purposes? Calling Update methods doesn't scale, imagine 5000 graphs being run every frame. The overhead on calling "Update" will already be significant (unity calls "Update" through reflection = slow), whatever code is in UpdateCSG will just make this worse
| if (!tree.Valid) | ||
| tree = CSGTree.Create(GetInstanceID()); | ||
| else | ||
| tree.Clear(); |
There was a problem hiding this comment.
At some point we need a new API where instead of you creating a tree here, it would create a branch, and this branch would be inserted in another tree. I'm working on redesigning this part of Chisel for this reason
There was a problem hiding this comment.
Do you mean like how I wrap a subgraph in a branch? Or like a global tree that contain everything?
There was a problem hiding this comment.
Each tree creates it's own separate meshes and they do not interact with each other, so in order to have a graph that can be used together with other brushes & generators, they need to be part of the same tree.
Right now the mechanism for that is less than ideal, so I'm redesigning it & after that the graph could just create a branch that can then be hooked into the tree that encompasses all generators and brushes
There was a problem hiding this comment.
Right, you are talking about making the graph instance work with chisel components, I wonder where should the integration goes, I would prefer keeping the node graph depend on the chisel core only. the use case I have is parametric modeling of a in game item, instead of level editing.
Making graph depends on component, or component depends on graph doesn't seem quite right. Maybe a third package "Graph Component" that depends on both packages?
There was a problem hiding this comment.
Well no, the components will work on top of chisel core as well. Just having one single graph to make everything is a bit limiting, so we need to have a solution where multiple graphs can each create pieces of a tree
|
|
||
| public static IEnumerable<Type> GetAllTypesDerivedFrom<T>() | ||
| { | ||
| #if UNITY_EDITOR && UNITY_2019_2_OR_NEWER |
There was a problem hiding this comment.
We don't support 2019_x so this could be removed
| foreach (var property in instance.properties) | ||
| using (new EditorGUILayout.HorizontalScope()) | ||
| { | ||
| DrawOverrideCheckbox(property); |
There was a problem hiding this comment.
It would be nice that you could set up within the graph which parameters to expose to the user, instead of exposing everything and using checkboxes to choose what to modify
There was a problem hiding this comment.
When the user add a property node, then it's supposed to be exposed. I will add a constant node for value inside graph only. If we are following unity's shader graph, we are supposed to have a blackboard, where user have a centralized location to set up all the exposed properties.
| { | ||
| public override void OnInspectorGUI() | ||
| { | ||
| var instance = target as ChiselGraphInstance; |
There was a problem hiding this comment.
You should either use the scriptableObject/scriptableProperties in ChiselGraphPropertyEditor.OnGUI(instance)
Or use [DisallowMultipleComponent] on ChiselGraphInstance
Because this will most definitely not work when multiple objects are selected at the same time
|
In order to make everything burst compatible, I am thinking the node graph should generate a run time format, which can be a array, with all the node in the graph packed as struct, then at runtime, user can only modify the node parameter of those struct. Then iterate all the elements in array inside burst, and get a csg tree |
Yeah it makes sense to have some sort of packed tree structure + packed data that contains all the values, that burst can just plow through really quickly |
2 participants
Related:
#94
My proof of concept node graph
https://github.com/mic-code/Chisel.Prototype/tree/node
Currently implemented feature,
Box brush
Cylinder brush
Subgraph
Float Property node for exposing parameter