Finish migrating tileset layers to classes #2346
Conversation
Using a more specific type is more correct.
layer_water is the first layer that loads its sprites on its own.
This is much easier to work with when drawing many sprites with offsets.
I also moved LAYER_WORKERTASK to a very similar class. Some deduplication will be needed.
Since many layers now load their sprites on their own, this lets us do it for all layers at once.
Multiple layers need to load sprites to represent unit activities. Reduce code duplication by loading them in an abstract base class.
So far only create the class. Tile loading and drawing will come next.
This is necessary to port the roads layer drawing to layer_roads.
This is a very deep refactor of the algorithms. The main change is the use of std::bitset to replace bool[], which provides a few helpful algorithms out of the box. Better code organization also helped reduce its complexity significantly.
This function used to drive the drawing of mapview layers, but has now been fully replaced by layer classes.
One variable is easier to work with than two.
Instead of trying to match something (with __ or _-_), skip the entry for the alt graphic.
Roads rely on having terrain information available, but the server sends terrains after rulesets. This moves extra initialization to the very end, in handle_rulesets_ready(). All tileset loading code shoud probably be done there.
This wraps load_sprite and specifies the tileset automatically, which is a nice syntactic sugar in layer classes.
It's a low-level function whose usage should ideally be limited to tilespec.cpp.
|
FWIW, I am not in favor of breaking this PR out into many. What I do think we might want to do is advertise this on Discord to see if other players that are not afraid of running pre-release code to put this through some varying tests. @blabber @hugomflavio come to mind. |
|
I'd like to see if there is a way to reduce the |
|
Sprite artifact in city dialog should be removed |
Fixes compilation with libc++.
I thought I could remove a warning about an optional sprite not being found but it doesn't seem to be the case.
When loading many optional sprites, it's better to log a single line summarizing everything than one line per sprite.
This will allow silencing them.
Also remove "warning" from the message itself.
Gate displaying these messages behind the --debug-tileset option. This reduces the client verbosity.
Looks bad with amplio2.
|
|
| @@ -1488,8 +1488,8 @@ static void tileset_add_layer(struct tileset *t, mapview_layer layer) | |||
| t, t->city_offset, t->city_flag_offset, t->occupied_offset)); | |||
| } break; | |||
| case LAYER_CITY2: { | |||
| t->layers.emplace_back( | |||
| std::make_unique<freeciv::layer_city_size>(t, t->city_size_offset)); | |||
| t->layers.emplace_back(std::make_unique<freeciv::layer_city_size>( | |||
There was a problem hiding this comment.
std library, need an include in header?
There was a problem hiding this comment.
It's only needed in the header if used there (which is the case). But vector is somehow magically included everywhere already, I'm not sure which header does that. If you want we can run iwyu in another PR...
There was a problem hiding this comment.
ok, i'll do a fresh compile on clang as a test as well
This concludes a refactoring of the tileset layer code initiated three years ago in #450. It doesn't mean that the tileset code is now perfect, but it's easier to work with. No user-facing change is expected.
Warning: very large PR. I think we need to discuss the merging strategy:
The idea is to remove the large
switch-based function that used to dispatch tile drawing and replace it with a series oflayer_xxxclasses, each of them responsible for one type of mapview layers. Dispatching to the correct class is done through inheritance. Since each class cares about a single layer, they can be small, making them easier to understand. It is also guaranteed that no other code fiddles with the class data.This PR removes about 2000 lines from
tilespec.cpp. What remains in that file is mainly the tileset loading logic including functions to cut out sprites from the tileset pngs, and many getter functions to query tileset properties.Migrating to classes required at least minor changes to the code to update sprite variable names. In many cases I also modified the logic to be easier to follow. An extreme example is
layer_roadswhich was basically rewritten from scratch.