Create PoC of SLAM functionality using Gaia by simone-gaia · Pull Request #11 · gaia-platform/examples-staging

simone-gaia · 2022-02-25T15:19:46Z

This example aims at teaching Gaia concepts using a real-world use-case scenario (SLAM). This is only for demonstration purposes and does not aim to be a real solution for SLAM.

Still, you can appreciate some of the Graph-based SLAM elements such as the graph data structure (graph, vertex, edge), observation of the external world (incoming_data_event), and building of the graph elements when new observations come in.

ghost · 2022-02-25T16:02:26Z

+-- license that can be found in the LICENSE.txt file
+-- or at https://opensource.org/licenses/MIT.
+----------------------------------------------------
+


These tables need an explainer - how do these get used?
I saw that there are comments in the ruleset that talks about hooking rules up to table events,
but "what is this table representing" should be described here as clearly as possible.
Especially the incoming_data_event which won't make sense reading here and is tough to follow from the ruleset.

Added a little explanation but not too detail since I'm no SLAM expert.

ghost · 2022-02-25T16:03:34Z

+    }
+
+    // Here's an example with slightly more processing. The paradigm here
+    //  is that incoming data has arrived, and that arrival in turn


What is this incoming data? From where? what does it mean?
Needs more explanation both here and in the ddl.

+1, would want to see a readme.md describing this, at a high level, and being able to relate the high-level with the low-level

Added explanation in the DDL:

-- This table represents a generic input from a sensor. -- i.e. Lidar sensor, image, etc.. -- Note: this is just an example.

ghost · 2022-02-25T16:04:24Z

+        }
+
+        // Create a vertex record
+        std::string graph_uuid_string("aaaaaaaa-0000-0000-0000-000000000000");


explain what the uuids are being used for

ghost

Needs a readme.MD to explain which files are important and set the stage for the example being presented. Not enough info here for the uninitiated to follow what is going on.

JackAtGaia · 2022-02-25T16:26:11Z

+BasedOnStyle: LLVM
+UseCRLF: false
+UseTab: Never
+IndentWidth: 4


in readme.md, should document that these are our styles, etc.

For the example repo, perhaps we can leave out our styles altoghether? If other people use this the styles are unlikely to align with theirs and it just adds complexity and extra build dependencies

JackAtGaia · 2022-02-25T16:27:49Z

+
+database gaia_slam
+
+table graph


+1 to what bill said. plus a good part in the readme.md detailing why, what, etc.

See comment above.

Added readme with pretty much same code content as the description of this PR.

JackAtGaia · 2022-02-25T16:30:13Z

+    {
+        std::stringstream ss;
+        ss << "Graph " << g.uuid << " vertices:\n";
+        for (g.vertices->v:vertex)


maybe I am a bit rusty, but cant we also do the print in a one-liner?

the reason I believe that is important is that they may do that accidentally, and they should know that it can happen, to look out for it.

JackAtGaia · 2022-02-25T16:33:52Z

+    gaia::system::initialize();
+
+    // This examples wants to focus on direct_access hence the rules are disabled.
+    gaia::rules::unsubscribe_rules();


why does it? requirements in readme.md?

what is this file trying to accomplish? what are the requirements? what is the task to perform?

why does it? requirements in readme.md?

Explained in the comment above.

/** * Showcase the usage of the direct_access API to create/read data from the database. * This example does not make usage of Rules. */

JackAtGaia · 2022-02-25T16:34:48Z

+    }
+
+    txn.commit();
+


how do we know this is working properly? is there expected output?

It prints the things specified in the gaia_log::app().info statements.

JackAtGaia · 2022-02-25T16:36:01Z

+
+    // We explicitly handle the transactions with begin_transaction() and commit_transaction()
+    // to trigger the rules.
+    gaia::db::begin_transaction();


why not move the begin and commit transactions inside of the calls? that is what I would expect.

I usually don't recommend it. It always happens that you then need to call the method from another method that already has a transaction open and then you have trouble.

JackAtGaia · 2022-02-25T16:36:44Z

+
+    gaia_log::app().info("=== Updates a vertex and observe the corresponding rules triggered ===");
+
+    // This code triggers the following rules:


the "this code triggers" is good. setting up what is expected, so they can follow along.

JackAtGaia

needs more work on describing what to do.

senderista · 2022-02-25T19:55:50Z

+cmake_minimum_required(VERSION 3.16)
+
+project(gaia_slam)
+project(gaia_slam)


senderista · 2022-02-25T19:56:15Z

+project(gaia_slam)
+project(gaia_slam)
+
+set(CMAKE_CXX_STANDARD 17)


Why C++17? Do we use C++17 features in this app?

Not sure, but I don't see a reason not to use it. (Well actually i do since our DAC headers use C++17 features, it's a known problem, better to proactively avoid warnings).

senderista · 2022-02-25T19:58:15Z

+    id uint64 unique,
+    type uint8,
+    data uint8[],
+    pose_x double,


This is an obvious candidate for a nested struct when we support complex types...

senderista · 2022-02-25T20:01:27Z

+    pose_y double,
+
+    -- Relationships
+    graph_uuid string,


Have we considered creating a special UUID type? This should be straightforward to implement as a flatbuffers struct with 2 64-bit ints, and it should be much faster than strings for comparisons. I'm worried about the potential perf penalty of using strings here (and in other usage of UUIDs). If enough apps need UUIDs then I think we should add a first-class UUID type, even if we don't generally support nested structs.

I will remove uuid for now. This is a leftover from the source example.

But yes, we should have first class uuid support.

senderista · 2022-02-25T20:02:46Z

+
+#include <gaia/logger.hpp>
+
+// Rule examples. These examples are in two categories. The first shows


wrapping is odd

What do you mean? If I had to change something I'd change to multiline comments /**/.

senderista · 2022-02-25T20:03:29Z

+        gaia_log::app().info("Inserted vertex record with ID {}", vertex.id);
+    }
+
+    // NOTE that having 2 on_insert rules for the same table results


should be either "NOTE:" or "Note that"

senderista · 2022-02-25T20:04:51Z

+
+
+    // Using tags
+    // Tags are a form of shorthand letting you use a shorter label


How about just "a tag is shorthand for a longer table name"?

senderista · 2022-02-25T20:05:45Z

+    // Record change (i.e., insertion or update)
+    on_change(edge)
+    {
+        // Fired for any insert or update to the edge table.


senderista · 2022-02-25T20:07:49Z

+
+    // Iterating through a 1:n relationship
+    //
+    // This example combines tags and following references between


"This example uses both tags and reference traversal to list the records in table B that are linked from table A due to changes in A."

senderista · 2022-02-25T20:08:07Z

+}
+
+
+// Rule examples where the creates other records.


"the rule"?

senderista · 2022-02-25T20:08:31Z

+
+
+// Rule examples where the creates other records.
+// The serial_group() keyword makes the rules in the ruleset to run serially:


"makes the rules in the ruleset run serially"

senderista · 2022-02-25T20:08:46Z

+
+// Rule examples where the creates other records.
+// The serial_group() keyword makes the rules in the ruleset to run serially:
+// only one rule is executed in a given point in time.


"only one rule is executed at a time"

senderista · 2022-02-25T20:11:48Z

+ * Wait an arbitrary amount of time for rule execution to terminate.
+ * Rules are triggered after commit and can take some time to fully execute.
+ */
+void wait_for_rules()


This is really documenting an antipattern that is sure to eventually bite users. If the timeout is too short, it will cause bugs. If it's too long, it will ruin performance.

If this antipattern is routinely needed to write nontrivial apps, then that's exposing a big hole in our API.

then that's exposing a big hole in our API.

I think this is the point. Nevertheless, the pattern is needed.

https://gaiaplatform.atlassian.net/browse/GAIAPLAT-2080

senderista · 2022-02-25T20:13:43Z

+
+    gaia_log::app().info("=== Retrieving vertexes/edges with type {} ===", vertex_type::c_image_keyframe);
+
+    // This code verify that the vertex with type vertex_type::c_image_keyframe has been created.


senderista · 2022-02-25T20:14:28Z

+    /// Now you can write your additional logic to trigger the other rules,
+    /// or write some new rules!
+    ///
+    /// For instance, you may want to create edges between vertexes..


you say "vertices" elsewhere, so be consistent...

senderista · 2022-02-25T20:15:43Z

+
+    if (vertex_it.begin() == vertex_it.end())
+    {
+        throw std::runtime_error("Impossible to find vertex with id 1 and type lidar_scan");


"Cannot find"

senderista · 2022-02-25T20:17:55Z

+        gaia_log::app().info(" Edge({}): ({}) --> ({})", e.id(), e.src().id(), e.dest().id());
+    }
+
+    txn.commit();


Better to enclose the auto_transaction_t in its own scope with all the mutating operations, so you don't need an explicit commit() (if you want to be explicit about the commit() for pedagogical reasons, you probably should use the low-level API rather than auto_transaction_t).

senderista · 2022-02-25T20:18:54Z

+
+    // We explicitly handle the transactions with begin_transaction() and commit_transaction()
+    // to trigger the rules.
+    gaia::db::begin_transaction();


Are you avoiding auto_transaction_t for pedagogical reasons (i.e. explicit begin/commit to make txn boundaries clear)?

senderista · 2022-02-25T20:19:22Z

+    // This code triggers the following rules:
+    // 1. on_insert(incoming_data_event): triggered when an incoming_data_event is inserted. This insert a new vertex.
+    // 2. All the rules triggered on vertex insertion.
+    gaia::db::begin_transaction();


Again, I would just use auto_transaction_t in a nested scope.

It would be just more verbose and add a level of indentation.

senderista · 2022-02-25T20:19:46Z

+    gaia_log::app().info("=== Retrieving vertexes/edges with type {} ===", vertex_type::c_image_keyframe);
+
+    // This code verify that the vertex with type vertex_type::c_image_keyframe has been created.
+    gaia::db::begin_transaction();


Again, I would just use auto_transaction_t in a nested scope.

Answered above.

Create PoC of SLAM functionality using Gaia

c846712

simone-gaia requested review from a user, JackAtGaia, LaurentiuCristofor, dr-soong, senderista, stevegaia, vDonGlover and zack4gaia February 25, 2022 15:19

ghost reviewed Feb 25, 2022

View reviewed changes

ghost suggested changes Feb 25, 2022

View reviewed changes

JackAtGaia reviewed Feb 25, 2022

View reviewed changes

JackAtGaia suggested changes Feb 25, 2022

View reviewed changes

senderista reviewed Feb 25, 2022

View reviewed changes

Address PR feedback

bafc2d3


		gaia_log::app().info("=== Updates a vertex and observe the corresponding rules triggered ===");

		// This code triggers the following rules:


		#include <gaia/logger.hpp>

		// Rule examples. These examples are in two categories. The first shows



		// Using tags
		// Tags are a form of shorthand letting you use a shorter label



		// Rule examples where the creates other records.
		// The serial_group() keyword makes the rules in the ruleset to run serially:


		gaia_log::app().info("=== Retrieving vertexes/edges with type {} ===", vertex_type::c_image_keyframe);

		// This code verify that the vertex with type vertex_type::c_image_keyframe has been created.

Conversation

simone-gaia commented Feb 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-soong Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackAtGaia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

senderista Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dr-soong Feb 25, 2022 •

edited

Loading

senderista Feb 25, 2022 •

edited

Loading