Skip to content

Conversation

@shevernitskiy
Copy link
Contributor

@shevernitskiy shevernitskiy commented Sep 1, 2023

This PR add reserved range feature to dynamic texture module.

Reserved range is a buffer in the textures.raws vector right after static game textures. Game will never wipes items of this range from the vector.

Now loadTilset(), loadTexture(), createTile(), createTileset() can receive additional optional parameter bool reserved which define target range (false by default, dynamic range).

close #3709

@shevernitskiy shevernitskiy requested a review from myk002 September 2, 2023 05:42
@shevernitskiy
Copy link
Contributor Author

I almost lost in logic branches:)

auto dummy_surface =
DFSDL_CreateRGBSurfaceWithFormat(0, 0, 0, 32, SDL_PixelFormatEnum::SDL_PIXELFORMAT_RGBA32);
for (int32_t i = 0; i < ReservedRange::size; i++) {
add_texture(dummy_surface);
Copy link
Member

Choose a reason for hiding this comment

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

at program close, DF will attempt to free all these textures. Is the refcount incremented somewhere so that it doesn't end up doing a double free?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

game will perform FreeSurface for entire textures.raws (i guess)... so every texture with refcount = 1 will be destroyed (here we destroy dummy)
our cached surfaces has refcount = 2 and it will be decrmented to 1..
then Texture::cleanup() will be called in Core shutdown sequence, there we will one more time call FreeSurface for our cached surfaces and finaly destroy it

Copy link
Member

Choose a reason for hiding this comment

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

I note that we currently double free on exit with current code in 50.09-r3, so this is a real issue. I'll test to make sure this PR fixes that issue before merging, but otherwise this is looking pretty good

@myk002
Copy link
Member

myk002 commented Sep 10, 2023

sorry for the conflicting changes in Textures.cpp -- df::global::enabler is NULL if we don't have the symbol map populated for a version, and the current usage was causing crashes and preventing us from investigating the upcoming 50.10 release

@myk002
Copy link
Member

myk002 commented Sep 10, 2023

There is still a double free going on here and DF is aborting when it frees the textures in its array. Could you double check the refcounting logic?

commenting out the for loop in reset_surface() avoids the double free

@shevernitskiy
Copy link
Contributor Author

shevernitskiy commented Sep 11, 2023

On a win there is no errors on close, they are visible only while debugging. That's why i missed that.
For me commenting out reset_surface() solves nothing, debugger still catches the exception.
Increasing refcount for dummy to equal size of reserved range eliminate the exception.

@myk002
Copy link
Member

myk002 commented Sep 24, 2023

could you possibly update this branch with latest changes from develop?

@shevernitskiy
Copy link
Contributor Author

rebased onto develop

@myk002 myk002 merged commit 0eba20d into DFHack:develop Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

textures become corrupted on game reload

2 participants