Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ & Python API: add helpers for constructing an entity path #4595

Merged
merged 10 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions crates/re_log_types/src/path/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub use parse_path::PathParseError;
/// Build a `Vec<EntityPathPart>`:
/// ```
/// # use re_log_types::*;
/// let parts: Vec<EntityPathPart> = entity_path_vec!("foo", "bar");
/// let parts: Vec<EntityPathPart> = entity_path_vec!("foo", 42, "my image!");
/// ```
#[macro_export]
macro_rules! entity_path_vec {
Expand All @@ -34,16 +34,19 @@ macro_rules! entity_path_vec {
::std::vec::Vec::<$crate::EntityPathPart>::new()
};
($($part: expr),* $(,)?) => {
vec![ $($crate::EntityPathPart::from($part),)+ ]
vec![ $($crate::EntityPathPart::from(
#[allow(clippy::str_to_string, clippy::string_to_string)]
$part.to_string()
),)+ ]
};
}

/// Build an [`EntityPath`] from parts that are _not_ escaped:
///
/// ```
/// # use re_log_types::*;
/// let path: EntityPath = entity_path!("world", "my image!");
/// assert_eq!(path, EntityPath::parse_strict(r"world/my\ image\!").unwrap());
/// let path: EntityPath = entity_path!("world", 42, "my image!");
/// assert_eq!(path, EntityPath::parse_strict(r"world/42/my\ image\!").unwrap());
/// ```
#[macro_export]
macro_rules! entity_path {
Expand Down
4 changes: 3 additions & 1 deletion crates/re_sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ pub use self::recording_stream::{

pub use re_sdk_comms::{default_flush_timeout, default_server_addr};

pub use re_log_types::{entity_path, ApplicationId, EntityPath, StoreId, StoreKind};
pub use re_log_types::{
entity_path, ApplicationId, EntityPath, EntityPathPart, StoreId, StoreKind,
};

pub use global::cleanup_if_forked_child;

Expand Down
19 changes: 15 additions & 4 deletions crates/re_sdk/src/recording_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -719,16 +719,17 @@ impl RecordingStream {
/// The data will be timestamped automatically based on the [`RecordingStream`]'s internal clock.
/// See [`RecordingStream::set_time_sequence`] etc for more information.
///
/// The entity path can either be a string
/// (with special characters escaped, split on unescaped slashes)
/// or an [`EntityPath`] constructed with [`crate::entity_path`].
/// See <https://www.rerun.io/docs/concepts/entity-path> for more on entity paths.
///
/// See also: [`Self::log_timeless`] for logging timeless data.
///
/// Internally, the stream will automatically micro-batch multiple log calls to optimize
/// transport.
/// See [SDK Micro Batching] for more information.
///
/// The entity path can either be a string
/// (with special characters escaped, split on unescaped slashes)
/// or an [`EntityPath`] constructed with [`crate::entity_path`].
///
/// # Example:
/// ```ignore
/// # use rerun;
Expand Down Expand Up @@ -791,6 +792,11 @@ impl RecordingStream {
/// internal clock.
/// See `RecordingStream::set_time_*` family of methods for more information.
///
/// The entity path can either be a string
/// (with special characters escaped, split on unescaped slashes)
/// or an [`EntityPath`] constructed with [`crate::entity_path`].
/// See <https://www.rerun.io/docs/concepts/entity-path> for more on entity paths.
///
/// Internally, the stream will automatically micro-batch multiple log calls to optimize
/// transport.
/// See [SDK Micro Batching] for more information.
Expand Down Expand Up @@ -830,6 +836,11 @@ impl RecordingStream {
/// All of the batches should have the same number of instances, or length 1 if the component is
/// a splat, or 0 if the component is being cleared.
///
/// The entity path can either be a string
/// (with special characters escaped, split on unescaped slashes)
/// or an [`EntityPath`] constructed with [`crate::entity_path`].
/// See <https://www.rerun.io/docs/concepts/entity-path> for more on entity paths.
///
/// Internally, the stream will automatically micro-batch multiple log calls to optimize
/// transport.
/// See [SDK Micro Batching] for more information.
Expand Down
33 changes: 33 additions & 0 deletions crates/rerun_c/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,39 @@ pub unsafe extern "C" fn rr_recording_stream_log(
}
}

// ----------------------------------------------------------------------------
// Private functions

#[allow(unsafe_code)]
#[no_mangle]
pub unsafe extern "C" fn _rr_escape_entity_path_part(part: CStringView) -> *const c_char {
let Ok(part) = part.as_str("entity_path_part") else {
return std::ptr::null();
};

let part = re_sdk::EntityPathPart::from(part).to_string(); // escape the part

let Ok(part) = CString::new(part) else {
return std::ptr::null();
};

part.into_raw()
}

#[allow(unsafe_code)]
#[no_mangle]
pub unsafe extern "C" fn _rr_free_string(str: *mut c_char) {
if str.is_null() {
return;
}

// Free the string:
unsafe {
// SAFETY: `_rr_free_string` should only be called on strings allocated by `_rr_escape_entity_path_part`.
let _ = CString::from_raw(str);
}
}

// ----------------------------------------------------------------------------
// Helper functions:

Expand Down
17 changes: 17 additions & 0 deletions crates/rerun_c/src/rerun.h
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,23 @@ extern void rr_recording_stream_log(
rr_recording_stream stream, rr_data_row data_row, bool inject_time, rr_error* error
);

// ----------------------------------------------------------------------------
// Private functions

/// PRIVATE FUNCTION: do not use.
///
/// Escape a single part of an entity path, returning an new null-terminated string.
///
/// The returned string must be freed with `_rr_free_string`.
///
/// Returns `nullptr` on failure (e.g. invalid UTF8, ore null bytes in the string).
extern char* _rr_escape_entity_path_part(rr_string part);

/// PRIVATE FUNCTION: do not use.
///
/// Must only be called with the results from `_rr_escape_entity_path_part`.
extern void _rr_free_string(char* string);
Comment on lines +425 to +430
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:
instead of doing the alloc/free dance a slightly safer alternative would be to use the "buffer in, count inout" pattern:

int rr_escape_entity_path_part(rr_string part, char* buffer, int buffer_size);

In this fairly common C pattern the desired count is returned but the buffer isn't fully written. Meaning one can either pass a null for buffer to just figure out the size one needs to allocate, or speculatively put in a buffer and check whether a second call is needed.

The advantage is that in many cases only one call is needed and that allocation stays with the caller.
The disadvantage is that when passing null buffer or too small buffer initially, this bottoms out in more work being done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't quite understand why new_entity_path is not fully implemented in the c interface

Copy link
Member Author

@emilk emilk Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this implementation because it was much faster to implement. But it's also why I made it private.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't quite understand why new_entity_path is not fully implemented in the c interface

I don't understand the question. new_entity_path returns a std::string, so cannot be defined in C.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could pass in several rr_string and write to a single char* buffer


// ----------------------------------------------------------------------------

#ifdef __cplusplus
Expand Down
4 changes: 4 additions & 0 deletions docs/code-examples/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ path = "depth_image_simple.rs"
name = "disconnected_space"
path = "disconnected_space.rs"

[[bin]]
name = "entity_path"
path = "entity_path.rs"

[[bin]]
name = "image_simple"
path = "image_simple.rs"
Expand Down
20 changes: 20 additions & 0 deletions docs/code-examples/entity_path.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Log a `TextDocument`

#include <rerun.hpp>

int main() {
const auto rec = rerun::RecordingStream("rerun_example_text_document");
rec.spawn().exit_on_failure();

rec.log(
R"(world/42/escaped\ string\!)",
rerun::TextDocument("This entity path was escaped manually")
);
rec.log(
rerun::new_entity_path({"world", std::to_string(42), "unescaped string!"}),
rerun::TextDocument("This entity path was provided as a list of unescaped strings")
);

assert(rerun::escape_entity_path_part("my string!") == R"(my\ string\!)");
assert(rerun::new_entity_path({"world", "42", "my string!"}) == R"(/world/42/my\ string\!)");
Wumpf marked this conversation as resolved.
Show resolved Hide resolved
}
11 changes: 11 additions & 0 deletions docs/code-examples/entity_path.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import rerun as rr

rr.init("rerun_example_entity_path", spawn=True)

rr.log(r"world/42/escaped\ string\!", rr.TextDocument("This entity path was escaped manually"))
rr.log(
["world", 42, "unescaped string!"], rr.TextDocument("This entity path was provided as a list of unescaped strings")
)

assert rr.escape_entity_path_part("my string!") == r"my\ string\!"
assert rr.new_entity_path(["world", 42, "my string!"]) == r"/world/42/my\ string\!"
16 changes: 16 additions & 0 deletions docs/code-examples/entity_path.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//! Example of different ways of constructing an entity path.

fn main() -> Result<(), Box<dyn std::error::Error>> {
let rec = rerun::RecordingStreamBuilder::new("rerun_example_text_document").spawn()?;

rec.log(
r"world/42/escaped\ string\!",
&rerun::TextDocument::new("This entity path was escaped manually"),
)?;
rec.log(
rerun::entity_path!["world", 42, "unescaped string!"],
&rerun::TextDocument::new("This entity path was provided as a list of unescaped strings"),
)?;

Ok(())
}
1 change: 1 addition & 0 deletions rerun_cpp/src/rerun.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "rerun/collection_adapter.hpp"
#include "rerun/collection_adapter_builtins.hpp"
#include "rerun/config.hpp"
#include "rerun/entity_path.hpp"
#include "rerun/error.hpp"
#include "rerun/recording_stream.hpp"
#include "rerun/result.hpp"
Expand Down
17 changes: 17 additions & 0 deletions rerun_cpp/src/rerun/c/rerun.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions rerun_cpp/src/rerun/entity_path.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#include "entity_path.hpp"

#include "c/rerun.h"
#include "error.hpp"
#include "string_utils.hpp"

namespace rerun {
std::string escape_entity_path_part(std::string_view unescaped) {
auto escaped_c_str = _rr_escape_entity_path_part(detail::to_rr_string(unescaped));

if (escaped_c_str == nullptr) {
Error(ErrorCode::InvalidStringArgument, "Failed to escape entity path part").handle();
return std::string(unescaped);
} else {
std::string result = escaped_c_str;
_rr_free_string(escaped_c_str);
return result;
}
}

std::string new_entity_path(const std::vector<std::string_view>& path) {
if (path.empty()) {
return "/";
}

std::string result;

for (const auto& part : path) {
result += "/";
result += escape_entity_path_part(part);
}

return result;
}

} // namespace rerun
19 changes: 19 additions & 0 deletions rerun_cpp/src/rerun/entity_path.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#pragma once

#include <string_view>
#include <vector>

namespace rerun {

/// Escape an individual part of an entity path.
///
/// For instance, `escape_entity_path_path("my image!")` will return `"my\ image\!"`.
std::string escape_entity_path_part(std::string_view str);

/// Construct an entity path by escaping each part of the path.
///
/// For instance, `rerun::new_entity_path({"world", 42, "unescaped string!"})` will return
/// `"world/42/escaped\ string\!"`.
std::string new_entity_path(const std::vector<std::string_view>& path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should take a rerun collection. That way you can also call it with a single string view without overloading

Then again maybe that's a disadvantage because it muddles with the semantics of "list of path parts" 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is plenty of room for improvement. I'd love to be able to call it with a mix of strings and integers to, and have the implementation call to_string on each non-string, but I feel this is good enough for now.


} // namespace rerun
2 changes: 2 additions & 0 deletions rerun_py/docs/gen_common_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ class Section:
"set_global_data_recording",
"set_thread_local_data_recording",
"start_web_viewer_server",
"escape_entity_path_part",
"new_entity_path",
],
class_list=["RecordingStream", "LoggingHandler", "MemoryRecording"],
),
Expand Down
14 changes: 12 additions & 2 deletions rerun_py/rerun_sdk/rerun/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,18 @@
"datatypes",
"disable_timeline",
"disconnect",
"escape_entity_path_part",
"experimental",
"get_application_id",
"get_data_recording",
"get_global_data_recording",
"get_recording_id",
"get_thread_local_data_recording",
"is_enabled",
"log",
"log_components",
"log",
"memory_recording",
"new_entity_path",
"reset_time",
"save",
"script_add_args",
Expand All @@ -92,7 +94,15 @@
import rerun_bindings as bindings # type: ignore[attr-defined]

from ._image import ImageEncoded, ImageFormat
from ._log import AsComponents, ComponentBatchLike, IndicatorComponentBatch, log, log_components
from ._log import (
AsComponents,
ComponentBatchLike,
IndicatorComponentBatch,
escape_entity_path_part,
log,
log_components,
new_entity_path,
)
from .any_value import AnyValues
from .archetypes import (
AnnotationContext,
Expand Down
Loading
Loading