Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1563 +/- ##
==========================================
- Coverage 51.98% 51.60% -0.38%
==========================================
Files 497 500 +3
Lines 27029 27303 +274
Branches 2512 2557 +45
==========================================
+ Hits 14051 14091 +40
- Misses 12978 13212 +234 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RiscadoA
left a comment
There was a problem hiding this comment.
Ty for working on this! Will be very very useful for online multiplayer too 👀
I'm not very familiar with the physics code, but I have two doubts regarding this implementation:
- why would
Interpolatedbe optional? Shouldn't it always be used? I don't see a reason to use physics without interpolation. - the whole interpolation mechanism should be independent from the physics plugin. It should be a separate plugin, outside the physics directory. Of course, the physics plugin would depend on it. This would allows us to use it for other stuff such as online multiplayer without necessarily enabling physics.
fallenatlas
left a comment
There was a problem hiding this comment.
Sorry for thanking so long to review and thank you for working on this! 🙏 Overall, I think this works well for what we want, I only have a couple concerns.
I think it reduces the necessity of the dev knowing that exists at the start and yeah, ideally we'd always want it I think. I guess it isn't for the same reasons as the solver and stuff also isn't included with the physics which, tbh, I think we should also change those as it doesn't make a lot of sense.
Having in account that fixed step is also not part of the physics plugin I think this would make sense yes. I'm worried about how it would work having multiple plugins writing to the same thing and having it work well. But, the biggest problem I see is that, this interpolation is strictly for fixedStep, if we had for example servers running outside of fixedStep I'm not sure if this plugin would be good for that. |
9568356 to
f045a50
Compare
RiscadoA
left a comment
There was a problem hiding this comment.
LGTM other than what I just pointed out!
|
|
||
| CUBOS_REFLECT_IMPL(cubos::engine::Interpolated) | ||
| { | ||
| return cubos::core::ecs::TypeBuilder<Interpolated>("cubos::engine::Interpolated").build(); |
There was a problem hiding this comment.
Any reason to not include reflection for the struct's fields?
f045a50 to
17379de
Compare
17379de to
11eb603
Compare
fallenatlas
left a comment
There was a problem hiding this comment.
Just a couple more things.
Also, I have a doubt. Do we have a guarantee that Position is nextPosition when a fixedStep starts? We need it to make sure that the physics remain independent from the framerate.
| /// @ingroup interpolation-plugin | ||
| /// @brief Interpolates position, rotation and scale of entities with the @ref Interpolated component in between | ||
| /// fixed updates. |
There was a problem hiding this comment.
Are these comments meant to be here this?
There was a problem hiding this comment.
What could be different? That there is no rotation interpolation yet?
11eb603 to
c3b4b44
Compare
c3b4b44 to
20c626c
Compare
| if (timer.value >= step.value) | ||
| { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
redundant boolean literal in conditional return statement
| if (timer.value >= step.value) | |
| { | |
| return true; | |
| } | |
| return false; | |
| return timer.value >= step.value; |
things
Description
Add Interpolated{} to entities to interpolate them.
Example in distance constraint sample.
Checklist