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

[no squash] Extract image generation/cache from texturesource.cpp and some refactoring #14446

Merged
merged 2 commits into from Mar 30, 2024

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Mar 8, 2024

Goal of the PR

How does the PR work?

  • Everything related to image generation/modification and source images caching moved to imagesource.h/cpp
  • Since those are the parts which can only be accessed from the main tread, it simplifies the tread safety code in texturesource.cpp, and imagesource.cpp doesn't need any mutex.
  • The remaining texturesource.cpp gets refactored, but it's mostly modernizing and formatting, so there shouldn't be much behavioral difference.
  • SourceImageCache does also get refactored a little bit, but the rest of imagesource.cpp stays the same, since it would be too much for this PR to also reactor it.
  • Information to hopefully make it easier to review:
    • IImage is an image object, which can be modified.
    • "source image" is an IImage loaded from a texture file without any modification.
    • ITexture is made from an IImage for the driver to use. It doesn't get modified.

Does this relate to a goal in the roadmap?

2.2 Internal code refactoring

To do

This PR is Ready for Review.

(The next step is to refactor imagesource.cpp.)
(In the future, it may be good to replace const string reverences by string views everywhere, but this would involve changing many files.)

How to test

  • See that the functions which moved to imagesource.cpp didn't change.
    (e.g. use git diff --color-moved <commit1> <commit2>)
  • Read the refactored code.

@wsor4035 wsor4035 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Mar 8, 2024
@Zughy Zughy added the Roadmap The change matches an item on the current roadmap. label Mar 8, 2024
src/client/imagegen.h Outdated Show resolved Hide resolved
src/client/imagegen.h Outdated Show resolved Hide resolved
src/client/texturesource.cpp Show resolved Hide resolved
@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Mar 10, 2024
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

forgot to review the second commit

src/client/texturesource.cpp Show resolved Hide resolved
src/client/imagegen.h Outdated Show resolved Hide resolved
src/client/texturesource.cpp Outdated Show resolved Hide resolved
src/client/texturesource.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 self-assigned this Mar 10, 2024
@cx384 cx384 marked this pull request as draft March 10, 2024 17:21
@cx384
Copy link
Contributor Author

cx384 commented Mar 17, 2024

Thanks for the review, I resolved everything and rebased.

It is ready for review again.

@cx384 cx384 marked this pull request as ready for review March 17, 2024 14:06
@sfan5 sfan5 added One approval ✅ ◻️ and removed Rebase needed The PR needs to be rebased by its author. labels Mar 17, 2024
@cx384
Copy link
Contributor Author

cx384 commented Mar 19, 2024

Rebased to include the changes which #14448 made.

@sfan5 sfan5 merged commit 673d249 into minetest:master Mar 30, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Roadmap The change matches an item on the current roadmap. >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants