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

Do not allocate memory to transform layer if image is already set as single-layer #8241

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Districh-ru
Copy link
Collaborator

This PR contains changes from #8149 and should be merged after (or instead of) it.

This PR twice reduces the size of the allocated memory for single-layer images (if a single-layer state is set before the memory allocation for the image) during the resize() and copy().

The fheroes2 GUI often uses image restores which are single-layer images, but for every such restorer the engine also allocates memory for the never used transform-layer. Such operations are also done when generating big View World image and even for the single-layer screen surface.

NOTE: if already existing double-layer image is made single-layer the transform layer stays untouched: memory reallocation for this case is not done not to waste CPU resources.

@Districh-ru Districh-ru added improvement New feature, request or improvement ui UI/GUI related stuff labels Jan 7, 2024
@Districh-ru Districh-ru added this to the 1.1.0 milestone Jan 7, 2024
@Districh-ru Districh-ru self-assigned this Jan 7, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/2)

src/engine/zzlib.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (2/2)

src/engine/image.cpp Outdated Show resolved Hide resolved
@Districh-ru Districh-ru marked this pull request as ready for review January 7, 2024 11:25
@ihhub
Copy link
Owner

ihhub commented Jan 13, 2024

Hi @Districh-ru , I actually like the changes but I am still concerned of forcing single layer images to have only have of the array as we might still miss some places in the code where we use single-layered images.

What do you think if we merge all changes in the code except the ones made in Image class and possibly in screen.cpp?

@ihhub ihhub modified the milestones: 1.1.0, 1.0.12 Jan 13, 2024
@Districh-ru
Copy link
Collaborator Author

Hi @Districh-ru , I actually like the changes but I am still concerned of forcing single layer images to have only have of the array as we might still miss some places in the code where we use single-layered images.

What do you think if we merge all changes in the code except the ones made in Image class and possibly in screen.cpp?

Hi @ihhub, we have #8149 that only removes writes to the transform layer for the single-layer images, but keeps memory allocation for it.
Or you mean that we should first apply all other changes including new assertions? I'll prepare a PR a little later with such changes.

But single-layer images are single-layer and if somewhere we access the transform layer for such images then it is wrong. And we should fix that places instead. Keeping the transform layer for such images just as a patch for cases when some function accidentally access the transform layer IMHO is not good. :)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/fheroes2/game/game_credits.cpp Outdated Show resolved Hide resolved
@ihhub ihhub modified the milestones: 1.0.12, 1.1.0 Feb 7, 2024
@ihhub ihhub modified the milestones: 1.1.0, 1.1.1 May 22, 2024
@ihhub ihhub modified the milestones: 1.1.1, 1.1.2 Jul 13, 2024
@ihhub ihhub modified the milestones: 1.1.2, 1.1.3 Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement New feature, request or improvement ui UI/GUI related stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants