Feature/flexible layer #22

nathanhhughes · 2024-11-22T18:44:31Z

This includes several major changes that I want to put in front of everyone sooner rather than later:

Replaces DynamicSceneGraphLayer with SceneGraphLayer and changes terminology: "dynamic" layers are now "partitions" of a layer (where the normal layers are partition 0)
Layers are no longer fixed at graph creation. You can add and remove layers as needed
Layers can be accessed by name; names are configurable. Layers can have multiple names.
I removed several scene graph methods that didn't make sense any more or were near duplicated of other functionality (e.g., setNodeAttributes was removed in favor of addOrUpdateNode)
SceneGraphNode no longer has a timestamp. Dynamics are being pushed to the attributes
Node emplacement has now changed to take an optional partition at the end of the list of arguments
Partitions are now just indexed by a plain integer (instead of the LayerPrefix class that could be constructed via a char). Most of the python functionality will still accept a char in place of an integer partition ID, but it'll map to the ascii value
The nodes and edges iterators over the scene graph iterate over ALL nodes (previously it used to only be static nodes)

I haven't updated Hydra, etc. yet so some of the API is still subject to change (I'm trying to use the python unit tests here and Hydra to make sure the API is sane / things don't change too much)

nathanhhughes · 2024-11-22T18:47:32Z

Last change that I forgot: There is no more IsolatedSceneGraphLayer: you can modify nodes and edges in the normal SceneGraphLayer (I previously disallowed this in case someone messed with the scene graph book-keeping by modifying a layer directly instead of modifying the scene graph, but I realized you would have to use const_cast to do this, which is a strong enough check against someone doing something silly)

Schmluk

Looks great! I think will make quite a few things nicer and easier to use!

Schmluk · 2024-11-22T19:29:34Z

include/spark_dsg/scene_graph_layer.h

+   * @param node_id node to get
+   * @returns a potentially valid node constant reference
+   */
+  const SceneGraphNode* findNode(NodeId node_id) const;


Minor: While we're at it, wanted to throw out the point of mutable/const access with pointer members. How much do we care about the scene graph actually not being modified (including node attributes etc) when it is constant? I know this requires more duplicate interfaces for const access but might make some designs (multi-threading, read-only) easier.

Schmluk · 2024-11-22T19:30:03Z

include/spark_dsg/scene_graph_layer.h

+  /**
+   * @brief Get a particular node in the layer
+   * @param node_id node to get
+   * @returns a potentially valid node constant reference


Can const refs be invalid?

Schmluk · 2024-11-22T19:31:27Z

include/spark_dsg/scene_graph_layer.h

   */
-  const SceneGraphNode* findNode(NodeId node_id) const override;
+  const SceneGraphEdge* findEdge(NodeId source, NodeId target) const;


Minor: It would also be useful to have a findEdges(src) function that gives all connectivity of a node. Maybe that is already implemented somewhere else though.

There's a connections() method that will get you the IDs of all the neighbors (but it is annoying to have to grab the edge attributes once you have those)

Schmluk · 2024-11-22T19:33:52Z

include/spark_dsg/scene_graph_layer.h

-   * @brief Get node id of deleted nodes
-   */
-  void getRemovedNodes(std::vector<NodeId>& removed_nodes, bool clear_removed) override;
+  void getNewNodes(std::vector<NodeId>& new_nodes, bool clear_new) const;


Is there a reason for getting the result by reference? I usually like std::vector<NodeId> newNodes(bool clear_new=false) better.

I agree, but these get used in the scene graph class to populate all the new nodes, etc. for every layer (so it feels slightly cleaner to fill an existing vector instead of returning a new one and inserting)

Schmluk · 2024-11-22T19:35:29Z

include/spark_dsg/scene_graph_node.h

@@ -70,29 +69,13 @@ class SceneGraphNode {
   * @param id the id of the node to create
   * @param layer the layer that the node will belong to
   * @param attrs attributes for the node
+   * @param timestamp Optional node timestamp in nanoseconds


Is this still accurate?

Schmluk · 2024-11-22T20:02:33Z

src/dynamic_scene_graph.cpp

  }

  return total_nodes;
 }

-size_t DynamicSceneGraph::numDynamicNodes() const {
+size_t DynamicSceneGraph::numUnpartitionedNodes() const {


What does unpartitioned nodes mean? This looks like just num nodes in DSG to me?

Schmluk · 2024-11-22T20:13:31Z

src/serialization/graph_binary_serialization.cpp

@@ -108,7 +120,7 @@ void parseEdge(const AttributeFactory<EdgeAttributes>& factory,

 void writeLayer(const SceneGraphLayer& graph, std::vector<uint8_t>& buffer) {
  BinarySerializer serializer(&buffer);
-  serializer.write(graph.id);
+  serializer.write(graph.id.layer);  // we don't save partition IDs


Is there a reason not to save these? Just thinking of confusion potential if partitions get re-shuffeled/indexed during I/O.

This serialization is distinct from the graph serialization. Layers can't be constructed standalone with a partition ID. I guess this would be a problem if you wanted to serialized a bunch of partitions from a graph independent of saving the graph (I think I punted on this because I didn't want to branch on file version)

* forward declare DynamicSceneGraph where possible * rework NodeSymbol and hide eigen in new file * forward declare node attributes and some bounding boxes * forward declare mesh for bounding box * forward declare edge attributes and remove graph utilities from layer * split bindings into separate files * push edges towards derived layers * (wip) move traits and status * split layer prefix from types * minor cleanup of node * simplify eraseVertices * drop getPosition and eigen dep * drop node symbol include * remove typedef from * add missing bindings and fix compilation * add binary serialization for python layers * fix pytorch tests * remove commented code --------- Co-authored-by: Nathan Hughes <[email protected]>

* try adding python workflow * add test deps * install torch cpu only * fix url typo * add python badge --------- Co-authored-by: Nathan Hughes <[email protected]>

* switch feature vector to single precision * bump version

* add room colormap and cleanup ironbow * add conversion to hsv and hls * move colormaps to be functions * move colormaps to new file * add 150 class map and divergent map * clean up discrete colormaps * better sort colormaps * improve divergent docstring --------- Co-authored-by: Nathan Hughes <[email protected]>

* add khronos object bindings * make complex types readonly for safety for now

* correctly parse flat meshes from old versions * Update src/node_attributes.cpp Co-authored-by: Lukas Schmid <[email protected]> --------- Co-authored-by: Nathan Hughes <[email protected]> Co-authored-by: Lukas Schmid <[email protected]>

* add mesh append function * expand mesh python bindings * add default args for transform

* recombine bindings and use system pybind11 * fix binding ordering for default args * revert back to fetch content and reformat cmake * revert changes for avoiding newer pybind features * conditionally enable zmq implementation * clean up dependencies and other changes * switch to submodule * enable verbose build * add more status * revert debugging and use recursive clone * install git before checking out code

* add graph metadata object * skip networkx tests if not available * add python metadata interface * add node and edge metadata * use nodesymbol as primary arg type and add implicit conversion * add node and edge metadata bindings

* fix ci build issue * update deprecated zmq functions

…json

nathanhhughes requested review from jaredstrader, GoldenZephyr, yunzc, marcusabate and Schmluk November 22, 2024 18:44

Schmluk approved these changes Nov 22, 2024

View reviewed changes

nathanhhughes and others added 11 commits December 12, 2024 21:23

Feature/python workflow (#11)

54c5425

* try adding python workflow * add test deps * install torch cpu only * fix url typo * add python badge --------- Co-authored-by: Nathan Hughes <[email protected]>

switch feature vector to single precision (#13)

fd5792f

* switch feature vector to single precision * bump version

add khronos object bindings (#15)

1752d6d

* add khronos object bindings * make complex types readonly for safety for now

Feature/mesh append (#17)

0d816aa

* add mesh append function * expand mesh python bindings * add default args for transform

remove implicit bool conversion (#19)

7ccd7c9

(fix) add validity check for layer view

862ef88

Feature/metadata (#21)

fb5a9b8

* add graph metadata object * skip networkx tests if not available * add python metadata interface * add node and edge metadata * use nodesymbol as primary arg type and add implicit conversion * add node and edge metadata bindings

nathanhhughes force-pushed the develop branch from 8de35eb to fb5a9b8 Compare December 12, 2024 21:24

nathanhhughes added 11 commits December 12, 2024 16:31

Fix/develop ci (#28)

0a8b0f2

* fix ci build issue * update deprecated zmq functions

change layer names to strings and clean up layer key

e286f23

drop getLayerKey function

043faef

change actions to only trigger on PR

6ea0285

flip LayerId to signed and update docstrings

0261cea

revise scene graph constructors to handle names

6072c4b

drop unused method

320ecde

start reworking get and has layer

b3e6dc9

(wip) remove dynamic layer

129f7ab

fix compilation issues from deleting dynamic layer

cffce3f

(wip) start dropping timestamp from nodes

090ef57

nathanhhughes added 27 commits December 12, 2024 21:33

fix scene graph tests and bump version

899b8f6

fix mergeGraph to use visitLayers

b4f5b68

check edge lookup validity

8d4a5b6

rename intralayer group to partition

3d6e63e

init partition layers with correct key

a7722bc

fix bug with field serialization order

77b9a30

touch up python tests to not use removed functions

bc3f620

start adding overload casts to bindings and serialize layer names in …

a053827

…json

fix infinite recursion and layer key comparison

cfeb01d

add string based layer constructor and fix other layer id issues

7452adb

update emplaceNode API and fix every test besides add remove

d4af3b3

fix graph node and edge iteration and networkx test invariants

5e2c259

add layer key comparison and unpartitioned counts

913c6bc

add hash for layer key and skip partitioned nodes in torch conversion

8e4b9f5

add unpartitioned iterators and fix torch graph conversions

9daaaa3

add timestamp to agent node and readd setNodeAttributes

6c70a7c

allow accessing partitions by name

4517701

update scene graph logger

13caa98

add layer key version of emplace

ae51df0

add name-based constructor

5c981e2

fix findEdge exception when nodes don't exist

4db809b

change getAttributesPtr to tryAttributes

29642b5

fix version warning and add new variant of addOrUpdateNode

12a050c

fix docstrings

fe99b5d

add and clean up scene graph documentation

e0a0280

fix mergeNodes error for partitioned nodes

ab8d426

deprecate getLabel

56363e3

nathanhhughes force-pushed the feature/flexible_layer branch from 338ff0f to 56363e3 Compare December 12, 2024 21:33

nathanhhughes force-pushed the develop branch from 0a8b0f2 to 0a8b29b Compare December 12, 2024 22:06

fix missing layer name serialization

483fecf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/flexible layer #22

Feature/flexible layer #22

nathanhhughes commented Nov 22, 2024

nathanhhughes commented Nov 22, 2024

Schmluk left a comment

Schmluk Nov 22, 2024

Schmluk Nov 22, 2024

Schmluk Nov 22, 2024

nathanhhughes Nov 22, 2024

Schmluk Nov 22, 2024

nathanhhughes Nov 22, 2024

Schmluk Nov 22, 2024

Schmluk Nov 22, 2024

Schmluk Nov 22, 2024

nathanhhughes Nov 22, 2024

Feature/flexible layer #22

Are you sure you want to change the base?

Feature/flexible layer #22

Conversation

nathanhhughes commented Nov 22, 2024

nathanhhughes commented Nov 22, 2024

Schmluk left a 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