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

Restructure tileset loading and storing. #22579

Merged
merged 55 commits into from Dec 12, 2017
Merged

Conversation

BevapDin
Copy link
Contributor

@BevapDin BevapDin commented Dec 9, 2017

Moves storing the loaded tileset data (the textures and the id-to-texture map) into a separate class tileset.
Mostly done to reduce the size of cata_tiles and to allow const-access to the loaded tileset data.

Moves loading the tileset into a separate class tileset_loader.
Mostly done to avoid forwarding all the meta data (used only during loading) via function parameters.

The behavior should be the same as before, except this:

  • Tile textures are stored as one (or more) large instance of SDL_Texture, instead of having one instance of that per tile. This is a bit more efficient, it only requires storing the offset and dimension of the tile along with the (shared) pointer to SDL_Texture for each tile.

  • The texture size respects the maximal supported texture size as reported by SDL. This may fix issue Fails to start with segfault and error: "Failed to map mcs buffer into GTT" #21317.

  • Loading the tileset is now done during loading the game data (but not if it was already loaded). This makes the game executable start a bit faster.


If this is too large, I can split it.

It does not use any of the class members.

Calls to it from within `cata_tiles` must use the fully qualified name
as it would otherwise attempt refer to the class member function of the
same name (but with different parameters).
If it's empty, its `pick` function will still have defined behaviour, it will just return a `nullptr`.
This is checked later anyway, so we don't need to check it before.

This fixes the later `return` statement: it was (normally) never triggered because `pick` only returns a `nullptr` when the weighted list is empty, but that case was handled above (in the now removed code).
There it used to return `true`, so it must still return `true` when the list is empty.
The whole class is only used when `use_tiles` is true, including any of its functions.
If one of them is called, `use_tiles` is certainly true, no need to check for it.
It's only used in a boolean context, and all calls to the function supply either 1 or 0 as value.
All kind of code that needs to get whether an entry with given id exists, or that needs to get the matching value from that map will use it now.
Get rid of the *repeated* magic string and the string manipulation.
…ileset

This also means they don't need to be cleared explicitly, because resetting the `tileset` pointer does this already.

Add some fancy getter functions.
…item_highlight

Instead of just creating the texture, it can now be called without checking for the existance of the texture.
The function will simply not do anything if the texture already exists.
This was done by the calling code previously.
This allows us to get rid of calling it during drawing.
It's now called from a function that itself already throws, so we can do the same here.
@Zireael07
Copy link
Contributor

Zireael07 commented Dec 9, 2017

What happens if texture size exceeds the max size specified by SDL? I mean, is the extra portion silently dropped or what?

@BevapDin
Copy link
Contributor Author

BevapDin commented Dec 9, 2017

What happens if texture size exceeds the max size specified by SDL? I mean, is the extra portion silently dropped or what?

It's split into several textures.

Add a reference to it to class `minimap_submap_cache` instead of using the global instance.

Explicitly delete the `cata_tiles` instance in `tilecontext` n `WinDestroy`. This will implicitly delete the `tex_pool` member and that will clear it, so calling `clear` (via `clear_texture_pool`) is not needed anymore.
It implicitly destroys and therefor clears the members, an explicit `clear` call is not needed at all.
Change `cata_tiles::init` into `cata_tiles::load_tileset`.
Store the id of the loaded tileset, so loading it again can be skipped.

It's called via the `DynamicDataLoader` now and when the tiles options have been changed.

Because it's down from the `DynamicDataLoader` after all the game data has been loaded, one can also call `do_tile_loading_report` here (it requires the game data to be loaded).
Starting the game requires knowing the tile dimensions to determine the window size.
Added code will now load the tile dimensions upon program start.
@kevingranade
Copy link
Member

This looks odd, have fun :)

Core was generated by `tests/cata_test'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 std::__uniq_ptr_impl<tileset, std::default_delete >::_M_ptr (this=0x8) at /usr/include/c++/7.2.0/bits/unique_ptr.h:147
147 pointer _M_ptr() const { return std::get<0>(_M_t); }
(gdb) bt
#0 std::__uniq_ptr_impl<tileset, std::default_delete >::_M_ptr (this=0x8) at /usr/include/c++/7.2.0/bits/unique_ptr.h:147
#1 std::unique_ptr<tileset, std::default_delete >::get (this=0x8) at /usr/include/c++/7.2.0/bits/unique_ptr.h:337
#2 std::unique_ptr<tileset, std::default_delete >::operator bool (this=0x8) at /usr/include/c++/7.2.0/bits/unique_ptr.h:351
#3 cata_tiles::load_tileset (this=this@entry=0x0, tileset_id="ChestHole", precheck=precheck@entry=false) at src/cata_tiles.cpp:195
#4 0x000055e5bdb66b7e in load_tileset () at src/sdltiles.cpp:1574
#5 0x000055e5bdb7f3fb in std::_Function_handler<void (), void (*)()>::_M_invoke(std::_Any_data const&) (__functor=...)
at /usr/include/c++/7.2.0/bits/std_function.h:316
#6 0x000055e5bce3c38a in std::function<void ()>::operator()() const (this=) at /usr/include/c++/7.2.0/bits/std_function.h:706
#7 0x000055e5bdb7b6da in DynamicDataLoader::finalize_loaded_data (this=0x55e5be3f8560 DynamicDataLoader::get_instance()::theDynamicDataLoader, ui=...)
at src/init.cpp:451
#8 0x000055e5bcde2e3a in game::load_world_modfiles (this=0x55e5bf47a7f0, world=0x55e5bf53e600, ui=...) at src/game.cpp:3783
#9 0x000055e5bccc168a in init_global_game_state (mods=std::__debug::vector of length 1, capacity 1 = {...}) at test_main.cpp:90
#10 0x000055e5bccc2650 in main (argc=, argv=) at test_main.cpp:150

@kevingranade
Copy link
Member

It's a bit more mundane than it looked at first, the change to init.cpp pulled tileset loading into the execution path for the test application, but the test application does not initialize SDL, so regardless of this particular failure, I wouldn't expect it to complete successfully.

The test application isn't making assertions about interactions with SDL, so there's no reason to have it do anything with it. Options include adjusting the test application to satisfy whatever requirements the tilecontext and friends have, or subverting the options to put the app into ascii mode.

If the SDL initialization has not been run, the `tilecontext` pointer has not been set, so we skip loading the tileset.
@Proxiehunter
Copy link

Game crash persists in build 7005

@Dyrewulfe
Copy link
Contributor

Dyrewulfe commented Jan 11, 2018

@BevapDin

I suspect there's a glitch in here somewhere. Relative hardware performance has improved compared to software(due to not having to constantly bind/unbind textures), but the overall drawing performance has decreased dramatically.

I'm running an Intel integrated, but it wasn't affected by #21317. The issue affects both Windows(OpenGL and Direct3D backends) and Linux builds, as well as my Android devices I've tested on.

Is there a logic error here causing unnecessary drawing, perhaps?

Edit: I'm asking because if there is, I'm not seeing it.

Also, the Android port UI elements are broken by this PR, which should not be the case as they're handled directly. My gut says the screen is being redrawn excessively.

@BevapDin
Copy link
Contributor Author

@Graywolfe813

Is there a logic error here causing unnecessary drawing, perhaps?

The change to drawing is this:

Previously each tile was stored in its own texture instance. The drawing code would draw the whole texture at once.
Now several tiles are stored together in one texture instance. The drawing code draws only part of the texture at once.

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.

None yet

5 participants