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

Split up engine.{h,cpp} #7625

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Split up engine.{h,cpp} #7625

merged 1 commit into from
Jan 9, 2025

Conversation

glebm
Copy link
Collaborator

@glebm glebm commented Jan 8, 2025

Untangles dependencies by splitting up engine.{h,cpp} into 3 files:

  1. engine/render/primitive_render.{hpp,cpp}
  2. engine/ticks.{hpp,cpp} -- only contains GetAnimationFrame for now.
  3. GetWidth2 renamed to CalculateSpriteTileCenterX and moved to levels/dun_tile.hpp.

GetDirection(Point, Point) is moved into point.

@glebm glebm enabled auto-merge (rebase) January 8, 2025 08:43
* Width2 is needed for savegame compatibility and to render animations centered
* @return Returns Width2
*/
constexpr int CalculateWidth2(int width)
Copy link
Member

@AJenbo AJenbo Jan 8, 2025

Choose a reason for hiding this comment

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

  • GetRenderingOffset()
  • CalculateHorizontalOffset()
  • CalculateCenterOffset()
  • CalculateSpriteCenter()

Move it to one of the rendering headers, probably primitive_render.cpp.

Copy link
Collaborator Author

@glebm glebm Jan 8, 2025

Choose a reason for hiding this comment

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

These are all kinda inaccurate because the function only works on sprite with width 64 (?). Is it OK if we leave this for a future PR?

We can then figure out a name, perhaps something like CalculateSpriteCenter64?
A rendering header also is not ideal because then loadsave would depend on rendering.

Copy link
Member

Choose a reason for hiding this comment

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

Considering that saveload is trying to stay compatible with a memory dump of a not to well thought out format I think that isn't to much of an issue.

It would be nice if we could resolve it as part of this PR as not to complicated and we have kept putting it off. This PR is dealing with where to put things and what to name them (at least the new places) so I don't think it's to out of scope either. CalculateSpriteCenter64 is an ok name.

Copy link
Collaborator Author

@glebm glebm Jan 8, 2025

Choose a reason for hiding this comment

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

I'm having trouble coming up with a good description for the function because I don't quite understand how it's used (I also have a flu so that could be why 🤧).

I don't think it should be in the rendering code (loadsave should not depend on rendering, it'd defeat the purpose of splitting things up), perhaps in util/sprite_width64.hpp or something?

Copy link
Member

Choose a reason for hiding this comment

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

i guess that would be ok. It doesn't need to be perfect, just a bit better then what's there :)

Copy link
Member

@AJenbo AJenbo Jan 8, 2025

Choose a reason for hiding this comment

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

Could the 64px be in reference to the tile size and not related to the actual sprite per say? Meaning it's finding the center of the sprite and placing it at the center of the tile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that might be it!

Renamed to CalculateSpriteTileCenterX and moved to levels/dun_tile.hpp.

Untangles dependencies by splitting up `engine.{h,cpp}` into 3 files:

1. `primitive_render`
2. `ticks` -- only contains `GetAnimationFrame` for now.
3. `GetWidth2` renamed to `CalculateSpriteTileCenterX` and moved to `levels/dun_tile.hpp`.
*/
constexpr int CalculateSpriteTileCenterX(int width)
{
return (width - TILE_WIDTH) / 2;
Copy link
Member

Choose a reason for hiding this comment

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

Nice :)

@glebm glebm merged commit c31836e into diasurgical:master Jan 9, 2025
23 checks passed
@glebm glebm deleted the mv-engine branch January 9, 2025 10:20
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.

2 participants