Chisel Graph #311

mic-code · 2021-01-26T03:08:17Z

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

LogicalError · 2021-01-26T08:12:54Z

Assets/ChiselNodes/Test.cs

+
+namespace Chisel.Core
+{
+    public class Test : MonoBehaviour


This should, of course, not be part of a non draft PR. But you already know that

LogicalError · 2021-01-26T08:14:11Z

...isel.components/Chisel/Components/API.public/Components/Containers/ChiselGeneratedObjects.cs

@@ -597,6 +597,7 @@ public bool HasLightmapUVs
                }
            }

+


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

Will clean, thanks for the catch

LogicalError · 2021-01-26T08:19:12Z

Packages/com.chisel.graph/Chisel/Nodes/PropertyNode.cs

+    public class PropertyNode<T> : Node, IPropertyNode where T : GraphProperty
+    {
+        [HideInInspector]
+        public T property;


shouldn't this be private?

LogicalError · 2021-01-26T08:19:50Z

Packages/com.chisel.graph/Chisel/Nodes/PropertyNode.cs

+    public abstract class GraphProperty
+    {
+        public string Name;
+        public bool overrideValue;


nitpicking, but override should start with upper case letter, just like Name

actually, makes more sense to make Name lowercase since they're both fields

LogicalError · 2021-01-26T08:23:12Z

Packages/com.chisel.graph/Chisel/ChiselGraph.cs

+    [CreateAssetMenu(fileName = "New Chisel Graph", menuName = "Chisel Graph")]
+    public class ChiselGraph : NodeGraph
+    {
+        public ChiselGraphNode active;


since the active node is set through SetActiveNode, wouldn't it make sense to make this either readonly ( get; private set; } or just private?

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

[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

LogicalError · 2021-01-26T08:23:32Z

Packages/com.chisel.graph/Chisel/ChiselGraph.cs

+        public ChiselGraphNode active;
+        public ChiselGraphInstance instance;
+
+        public List<GraphProperty> properties;


same for properties

LogicalError · 2021-01-26T08:24:48Z

Packages/com.chisel.graph/Chisel/ChiselGraph.cs

+    public class ChiselGraph : NodeGraph
+    {
+        public ChiselGraphNode active;
+        public ChiselGraphInstance instance;


But it seems that a graph is tied 1:1 to an instance? I guess this is a temporary thing to get started?

Can have multiple instance using the same graph, but you are right, temporary

LogicalError · 2021-01-26T08:30:21Z

Packages/com.chisel.graph/Chisel/ChiselGraphInstance.cs

+
+            for (int i = 0; i < graph.properties.Count; i++)
+                if (graph.properties[i].Name != properties[i].Name)
+                    InitProperties();


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

Will hashset be better? btw anyway to sync the properties without destroying existing properties value?

Is it possible to set a flag when a name changes instead? Or at least limit this to when editing the graph itself somehow?

LogicalError · 2021-01-26T08:32:04Z

Packages/com.chisel.graph/Chisel/ChiselGraphInstance.cs

+        public Dictionary<string, GraphProperty> overriddenProperties;
+
+        CSGTree tree;
+        List<Mesh> meshes;


can be
readonly List meshes = new List()
and you can remove the "meshes = new List();" in Start below

same goes for properties/overriddenProperties

LogicalError · 2021-01-26T08:34:22Z

Packages/com.chisel.graph/Chisel/ChiselGraphInstance.cs

+
+        void Update()
+        {
+            UpdateCSG();


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

LogicalError · 2021-01-26T08:38:17Z

Packages/com.chisel.graph/Chisel/ChiselGraphInstance.cs

+            if (!tree.Valid)
+                tree = CSGTree.Create(GetInstanceID());
+            else
+                tree.Clear();


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

Do you mean like how I wrap a subgraph in a branch? Or like a global tree that contain everything?

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

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?

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

LogicalError · 2021-01-26T08:42:52Z

Packages/com.chisel.graph/Chisel/Editor/ChiselGraphInstanceEditor.cs

+
+        public static IEnumerable<Type> GetAllTypesDerivedFrom<T>()
+        {
+#if UNITY_EDITOR && UNITY_2019_2_OR_NEWER


We don't support 2019_x so this could be removed

LogicalError · 2021-01-26T08:45:23Z

Packages/com.chisel.graph/Chisel/Editor/ChiselGraphInstanceEditor.cs

+                foreach (var property in instance.properties)
+                    using (new EditorGUILayout.HorizontalScope())
+                    {
+                        DrawOverrideCheckbox(property);


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

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.

ok, fair enough

LogicalError · 2021-01-26T09:00:38Z

Packages/com.chisel.graph/Chisel/Editor/ChiselGraphInstanceEditor.cs

+    {
+        public override void OnInspectorGUI()
+        {
+            var instance = target as ChiselGraphInstance;


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

mic-code · 2021-01-26T09:02:42Z

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

LogicalError · 2021-01-26T09:05:07Z

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

Michael Yuen added 25 commits January 18, 2021 18:43

Update Unity version

997dea1

Added node package

3e82a59

Added ChiselGraph and ChiselGraphInstance

43ed055

Added ChiselGraphNode editor

1ef1a6b

WIP

6dab7d5

WIP

697fefd

First working cube

2cbf7c9

Implemented BoxNode

0313600

Implemented CSG operation

5dc4fe7

Cleaned up

5da06c9

Implemented SubGraph

3ce6297

Implemented FloatPropertyNode

2f344ef

Hide property in GraphInstance

bde82cd

Implemented overridable editor GUI

9424b35

Implemented overridable properties

0363177

Handle multiple instance

834dac9

Property create and destroy update

3f1155a

Renamed to chisel graph

e131813

Added local position and rotation

b940f9a

Treat SubGraphNode like other node

9176bd1

Added ChiselGraphHierarchyView

3af4683

Show position and rotation in ChiselGraphHierarchyView

3e8d8d5

Clear only when dirty

ab6d4dc

Allow selecting any obj in ChiselGraphHierarchyView

fac4364

Implemented cylinder node and multiple input connections

e5835b0

LogicalError reviewed Jan 26, 2021

View reviewed changes

GraphRuntime WIP

2ff0cb6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chisel Graph #311

Chisel Graph #311

mic-code commented Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 27, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

mic-code Jan 26, 2021

LogicalError Jan 26, 2021

LogicalError Jan 26, 2021

mic-code commented Jan 26, 2021

LogicalError commented Jan 26, 2021

Chisel Graph #311

Are you sure you want to change the base?

Chisel Graph #311

Conversation

mic-code commented Jan 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mic-code commented Jan 26, 2021

LogicalError commented Jan 26, 2021