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.

Sign up for GitHub

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

Jump to bottom

Pre-parsed paths for GetPath functions #4080

Closed
james7132 opened this issue Mar 2, 2022 · 0 comments
Closed

Pre-parsed paths for GetPath functions #4080

james7132 opened this issue Mar 2, 2022 · 0 comments
Labels
A-Animation Make things move and change over time A-Reflection Runtime information about types C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times

Comments

@james7132
Copy link
Member

james7132 commented Mar 2, 2022 •
edited
Loading

What problem does this solve or what need does it fill?

Being able to access &(mut) dyn Reflect fields from a larger dyn Reflect via GetPath is a very nice QOL option when loading scenes or just looking for a runtime reflection. However, repeatedly calling it's functions with the same string is wasteful as it re-parses the same string at every call.

What solution would you like?

It'd be ideal to have alternative APIs for parsing a field path ahead of time, and just using the parsed token stream or access-list.

Ideally this would also look up type registrations, possibly using static type information (#4042) to look up field numbers instead of using string based lookups to help further cut down on string operations on this fast-path.

What alternative(s) have you considered?

  • Manually implementing a token-stream based equivalent to path, path_mut, get_path, and get_path_mut. This would work for this one use case, but wouldn't be generically available as a fast-path for other use cases.
  • Pseudo-JIT compilation for creating runtime lenses for extracting references for a given type. This would probably be the most efficient way to do so, but also the most complex.

Additional context

This is an optional, but highly recommended optimization for #4026 to enable faster application of animated values.

The current code already tokenizes individual parts of the path. We'd just need to decouple this into a separate 'static token stream that can be used as an alternative input.
@james7132 james7132 added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled A-Animation Make things move and change over time A-Reflection Runtime information about types C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels Mar 2, 2022
@james7132 james7132 mentioned this issue Mar 2, 2022
Closed
@MrGVSV MrGVSV mentioned this issue Jan 21, 2023
Closed
2 tasks
@bors bors bot closed this as completed in 8cd59b6 Jan 22, 2023
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@james7132 @MrGVSV @ItsDoot
bevy_reflect: Pre-parsed paths (bevyengine#7321)
69d5d7e 
# Objective

> ℹ️ **This is an adoption of bevyengine#4081 by @james7132**

Fixes bevyengine#4080.

Provide a way to pre-parse reflection paths so as to avoid having to parse at each call to `GetPath::path` (or similar method).

## Solution

Adds the `ParsedPath` struct (named `FieldPath` in the original PR) that parses and caches the sequence of accesses to a reflected element. This is functionally similar to the `GetPath` trait, but removes the need to parse an unchanged path more than once.

### Additional Changes

Included in this PR from the original is cleaner code as well as the introduction of a new pathing operation: field access by index. This allows struct and struct variant fields to be accessed in a more performant (albeit more fragile) way if needed. This operation is faster due to not having to perform string matching. As an example, if we wanted the third field on a struct, we'd write `bevyengine#2`—where `#` denotes indexed access and `2` denotes the desired field index.

This PR also contains improved documentation for `GetPath` and friends, including renaming some of the methods to be more clear to the end-user with a reduced risk of getting them mixed up.

### Future Work

There are a few things that could be done as a separate PR (order doesn't matter— they could be followup PRs or done in parallel). These are:

- [x] ~~Add support for `Tuple`. Currently, we hint that they work but they do not.~~ See bevyengine#7324
- [ ] Cleanup `ReflectPathError`. I think it would be nicer to give `ReflectPathError` two variants: `ReflectPathError::ParseError` and `ReflectPathError::AccessError`, with all current variants placed within one of those two. It's not obvious when one might expect to receive one type of error over the other, so we can help by explicitly categorizing them.

---

## Changelog

- Cleaned up `GetPath` logic
- Added `ParsedPath` for cached reflection paths
- Added new reflection path syntax: struct field access by index (example syntax: `foo#1`)
- Renamed methods on `GetPath`:
  - `path` -> `reflect_path`
  - `path_mut` -> `reflect_path_mut`
  - `get_path` -> `path`
  - `get_path_mut` -> `path_mut`

## Migration Guide

`GetPath` methods have been renamed according to the following:
- `path` -> `reflect_path`
- `path_mut` -> `reflect_path_mut`
- `get_path` -> `path`
- `get_path_mut` -> `path_mut`


Co-authored-by: Gino Valente <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Animation Make things move and change over time A-Reflection Runtime information about types C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times
Projects
Reflection
Status: Done
Milestone
No milestone
1 participant
@james7132