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

Add frame timings #1211

Merged
merged 15 commits into from
Jan 9, 2024
Merged

Add frame timings #1211

merged 15 commits into from
Jan 9, 2024

Conversation

pierd
Copy link
Contributor

@pierd pierd commented Dec 6, 2023

Addresses #1121

Makes input to rendered available in the debugger.

@pierd pierd requested review from philpax and ten3roberts December 6, 2023 14:29
Copy link
Contributor

@ten3roberts ten3roberts left a comment

Choose a reason for hiding this comment

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

Left some comments and questions 😊

Copy link
Contributor

@philpax philpax left a comment

Choose a reason for hiding this comment

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

LGTM outside of what Freja mentioned.

We might want to get timings for individual systems at some point int he future, but there's no particular rush. The breakdown you've done so far should be enough to get some useful information! Nice work :)

@philpax
Copy link
Contributor

philpax commented Dec 18, 2023

Shall we merge this?

@pierd
Copy link
Contributor Author

pierd commented Dec 18, 2023

Shall we merge this?

I'm going to update the structure a bit to make packages less coupled but that'll be after the Xmas break.

As for the coupling itself.. there'll still be coupling around the reporting api (whatever the report method would it be other parts have to know how (currently report_event on ambient_timings::Reporter) and what to report (currently TimingEvent/TimingEventType)). Using &'static str just makes the coupling implicit instead of explicit.

@ten3roberts ten3roberts self-requested a review January 9, 2024 15:42
Copy link
Contributor

@ten3roberts ten3roberts left a comment

Choose a reason for hiding this comment

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

Looks great,

I suppose we could say it is a good timing to merge this 😛

@pierd pierd merged commit 8bf5946 into main Jan 9, 2024
14 checks passed
@pierd pierd deleted the kuba/timings branch January 9, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants