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

Conversation

emilk
Copy link
Member

@emilk emilk commented Dec 20, 2023

What

Added rr.new_entity_path, which constructs an entity path from a list of strings.
rr.log also accepts such a list of unescaped strings directly.

In C++, rerun::new_entity_path has been added, e.g. rerun::new_entity_path({"world", "unescaped string!"}).

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

Added `rr.escape_entity_path`, which constructs an entity
path from a list of strings.

`rr.log` also accepts such a list of unescaped strings directly.
Copy link
Contributor

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

Maybe there is an opportunity to go for the more modern and familiar pathlib.Path approach:

rr.EntityPath("world") / "robot"
rr.EntityPath.root() / "world" / file_name / "some string"

Following this thread, and like pathlib.Path, log operations could be added as helper method:

entity = rr.EntityPath("world") / "camera"
entity.log(rr.Points3D(...))

Overall, this would give a very Pythonic look:

world = rr.EntityPath("world")
world.log(rr.Transform(....)

for stuff in my_world:
    entity = world / stuff.name()
    entity.log(rr.Points3D(...))

@emilk
Copy link
Member Author

emilk commented Dec 20, 2023

I like it, but create an issue for it - it is a much bigger task

@abey79
Copy link
Contributor

abey79 commented Dec 20, 2023

I like it, but create an issue for it - it is a much bigger task

#4596

@emilk emilk changed the title Python API: add helpers for constructing an entity path C++ & Python API: add helpers for constructing an entity path Dec 20, 2023
@emilk emilk added the 🌊 C++ API C or C++ logging API label Dec 20, 2023
@Wumpf Wumpf self-requested a review December 20, 2023 09:06
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

roundtrip test for C++ needs activating

Otherwise noted a few things I might have done differently on the interfaces, but not a strong preference so up to you 👍

crates/rerun_c/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +425 to +430
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);
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

docs/code-examples/roundtrips.py Outdated Show resolved Hide resolved
///
/// 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.

@emilk emilk merged commit c81d55e into main Dec 20, 2023
43 checks passed
@emilk emilk deleted the emilk/python-escape-entity-path branch December 20, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌊 C++ API C or C++ logging API include in changelog 🐍 Python API Python logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants