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

Click recording://entity/path links in markdown #3389

Closed
wants to merge 32 commits into from

Conversation

nikolausWest
Copy link
Member

@nikolausWest nikolausWest commented Sep 20, 2023

What

You can now embed links to entities in markdown documents using recording://entity/path, and link to components using recording://entity/path.Color.

In order to support this, a new DataPath is introduced, with a stricter parsing of entity paths. Previously foo bar was a valid path, but now you are only allowed ASCII, numbers, underscore and dash as names (outside of quotes).

A (uncompleted) description of the SfM example as markdown to

  • Test how that feels as documentation
  • Try out the ability to link directly to entities and components from within the markdown

image

image

Checklist


### Colored 3D Points
The colored 3D points were added to the scene by logging the
[rr.Points3D archetype](https://www.rerun.io/docs/reference/data_types/point3d)
Copy link
Member Author

Choose a reason for hiding this comment

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

Would have loved to be able to write

[`rr.Points3D` archetype](https://www.rerun.io/docs/reference/data_types/point3d)

here instead to make the rr.Points3D be formatted as both code and a link but not super important.

@nikolausWest
Copy link
Member Author

Would be super cool to also be able to refer to a specific time point but I'd need input on what that uri should look like

@nikolausWest nikolausWest requested a review from emilk September 20, 2023 15:02
@nikolausWest nikolausWest added the examples Issues relating to the Rerun examples label Sep 20, 2023
@emilk emilk changed the title Description markdown for SfM example Click recording://entity/path links in markdown Sep 20, 2023
Of course you can also have [normal https links](https://github.com/rerun-io/rerun), e.g. <https://rerun.io>.

## Image
![A random image](https://picsum.photos/640/480)
Copy link
Member Author

Choose a reason for hiding this comment

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

Something tells me it would be nicer to not add randomness to these but probably no big deal

@emilk emilk marked this pull request as ready for review September 21, 2023 13:46
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Someone else should review this, since I wrote most of the code in it 😬

@Wumpf Wumpf self-requested a review September 21, 2023 21:38
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.

oof, that was a lot of new path handling to take in, but looked all sound to me :)

Given that path parsing is quite central, I'm advocating for filling a few test gaps there.

now we need a Rerun markdown quine 😄

/// * `points.Color`
/// * `points[#42]`
/// * `points[#42].Color`
pub struct DataPath {
Copy link
Member

Choose a reason for hiding this comment

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

just noticed that InstancePath is the odd one out - it's in re_data_store unlike the others and has an overlapping relationship with DataPath. Might be nice to put them all next to each other in a follow-up PR and have an overview over all types of paths.

Copy link
Member

Choose a reason for hiding this comment

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

It is odd because it depends on InstanceKey which is a code-genned thing that lives in re_types. I guess we could move all paths to re_types though at some point soon.

crates/re_log_types/src/path/parse_path.rs Show resolved Hide resolved
crates/re_log_types/src/path/parse_path.rs Show resolved Hide resolved
@Wumpf
Copy link
Member

Wumpf commented Sep 22, 2023

Can we come up with something that makes it easy to distinguish weburls and internal urls on first sight? 🤔
Looking at this I wouldn't ever asssume that points entity is internal
image

Relatedly, our design makes it really hard to see what is a url and what is just bold faced :(
image

@emilk emilk force-pushed the niko/markdown-example-with-data-link branch from 44e6b22 to 2902399 Compare September 22, 2023 13:54
@emilk
Copy link
Member

emilk commented Sep 22, 2023

Styling links based on the URI is not really possible at this point

@emilk
Copy link
Member

emilk commented Sep 23, 2023

I'm a bit hesitant merging this, as it will break existing code.

I'm leaning towards implementing FroStr for EntityPath in a much more lenient way (basically same as on main) that will automatically convert /Hallå Där to /"Hallå Där"

@Wumpf
Copy link
Member

Wumpf commented Sep 23, 2023

if we want to disallow /Griaß di in the long run we could also issue a warning (_once) when converting it to /"Griaß di".

If on the other hand we make this defined behavior, we might need to take special care that we firmly establish /"Griaß di" == /Griaß di for all usages. But since all parsing goes through this that should be a given, right? 🤔
As I understand this now, quotes would be only really necessary in situations where we struggle delimiting/tokenizing the path parts, right?

@emilk
Copy link
Member

emilk commented Sep 23, 2023

The core problem is that foo.bar used to be a valid entity-path, but if we try to parse recording://foo.bar we cannot figure out "Is this the entity-path foo.bar, or is it the component path with entity-path=foo and component=bar?

So we either need to forbid . outside of quotes, or use different parsing for things we know are entity paths and things we don't know yet (URIs)

@emilk emilk added the do-not-merge Do not merge this PR label Sep 25, 2023
@emilk emilk removed the do-not-merge Do not merge this PR label Sep 25, 2023
@emilk
Copy link
Member

emilk commented Sep 25, 2023

Ok, parsing entity paths is very forgiving again, but logs a warning when not written in their "proper" form.

We could consider making entity parsing forgiving of any error, thus solving #3393 by simply allowing anything as an entity path.

# Conflicts:
#	crates/re_types/source_hash.txt
# Conflicts:
#	crates/re_types/source_hash.txt
@emilk
Copy link
Member

emilk commented Sep 25, 2023

something went wrong when merging.

Rebased in #3442

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Issues relating to the Rerun examples include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants