Skip to content

Conversation

ambeeeeee
Copy link
Contributor

With the recent changes to APIs in Time and Timer, I came across Touch which would benefit from the same change. Upon code approval I can add it to the changelog.

My last PR randomly killed itself when I was messing with the git history, odd.

@memoryruins memoryruins added C-Code-Quality A section of code that is hard to understand or change A-Input Player input via keyboard, mouse, gamepad, and more labels Nov 29, 2020
Copy link
Member

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

Seems consistent with the Timer changes.

Should delta and distance also be marked #[inline] for consistency?

@ambeeeeee
Copy link
Contributor Author

Seems consistent with the Timer changes.

Should delta and distance also be marked #[inline] for consistency?

I decided not to immediately since they're not simple getters and setters, but we'll see what @cart thinks.

@cart
Copy link
Member

cart commented Dec 1, 2020

Yeah whether or not to inline is always an interesting question. I think we should only "always default" to inlining simple field accesses. Everything else should probably be informed by benchmarks (both perf and compile time).

@cart cart merged commit 4c1bc02 into bevyengine:master Dec 1, 2020
@CleanCut
Copy link
Member

CleanCut commented Dec 1, 2020

Yeah whether or not to inline is always an interesting question. I think we should only "always default" to inlining simple field accesses. Everything else should probably be informed by benchmarks (both perf and compile time).

Great, it's nice to have a guideline. 👍

@fopsdev fopsdev mentioned this pull request Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Input Player input via keyboard, mouse, gamepad, and more C-Code-Quality A section of code that is hard to understand or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants