Instead of a pointer to a member of a derived class, used a virtual function implemented
in both derived classes (game_display and editor_display) that return an reference to the
appropriate object.
Also removed the overlay.hpp include from display.hpp. We can forward-declare the overlay
struct.
(cherry-picked from commit e583c470be)
They're still in the game_config namespace but are now in a more logical header.
game_config::version has been replaced with wesnoth_version.str(), save for one case
where it was replaced with wesnoth_version directly (it was a comparison against another
version_info object; no need to compare against a string...).
Also cleaned up a bunch of game_config.hpp includes.
(cherry-picked from commit b8d051cb72)
In particular, this allows the player to choose the filename before saving,
including the extension. If support for target format is present, Wesnoth
honors the extension and saves the screenshot in that format.
I also changed the default screenshot format back to PNG due to quality
issues JPEG has with pixel art. See discussion in pull request #2655.
This commit doesn't introduce any string changes. I used existing strings
for everything (sometimes having to take them from another textdomain).
The only function that display meaningfully implemented was get_disp_context. It did implement
get_tod_man, but only meaningfully in its two derived classes (it returned the result of
resources::tod_manager in display). Additionally, the long-ass comment in the header stated the
display class should *not* inherit from filter_context. Do note that get_disp_context was retained
as a display member function.
Upon further investigation, I found that there were only two places where the display class was
passed to unit_filter. All others passed resources::filter_con. The editor_display class was also
assigned to resources::filter_con in editor::context_manager. So, I removed the second filter_context
argument from unit_filter and initialized it from resources::filter_con in all cases.
unit_filter only used the filter_context to get its display_context and the unit_map within anyway.
When using display::get_disp_context in-game, that would return a game_board object. Using
resources::filter_con in the same context would return the game_state object that owned the
aforementioned game_board, and calling get_disp_context on that game_state would return the board
as well. So we end up in the same place.
To be complete, I also made editor::context_manager inherit from filter_context. It makes much more
sense, since it can nicely implement 2/4 of the fc functions. It also ensures resources::filter_con
is valid in the editor, in case it's needed. I'm not 100% sure it is, but since it was assigned
before (to editor_display), it's very likely.
Another side effect of the above is that get_tod_man is now implemented in a more apropos class
in the editor. Essentially, editor_display (where it was implemented before by reaching into the
context_manager) holds an editor_controller which holds a context_manager (where it's now implemented).
It wasn't even really needed in editor_display and was only called once.
Similarly, get_tod_man has been removed from game_display. Again, it was only present to "properly"
implement the function in display, which only existed because display inherited from filter_context.
And get_tod_man wasn't even needed in display, game_display, or editor_display (save for the one
aforementioned call)!!! AND in game_display, it simply dereferenced a pointer to the *actual* ToD
manager in the game_state class, WHICH IN AND OF ITSELF INHERITS FROM FILTER_CONTEXT!!!
I have removed the tod_manager pointer in game_display and replaced its use with resources::tod_manager,
which in the context of the game (where game_display) would be use, point to the same thing the class
pointer did. I suppose I could have left it, since I'm trying to remove/improve the use of the
resources pointers, but I felt it better to decouple the two classes (game_display and game_state)
slightly, especially since its main purpose for existing (overriding display::get_tod_man) no longer
exists.
Finally, some of the instances where a unit_filter object is created had to be changed to use brace
initialization or else the build failed. @celticminstrel tells me this might be because of some "most
vexing parse" issue.
Some formatting and virtual/override specifiers were also applied/added.
*huffs*
* Removed display::screen_area(), display::w(), and display::h().
* Moved the global screen_area() function into the CVideo class.
* Renamed CVideo::getx() and gety() to get_width() and get_height()
* Made those two functions return the result of screen_area() instead of the other way around.
* Added preliminary support for high-DPI rendering to screen_area()
Note on the last point: When I fixed bug #1772 (aa8f6c7e7 right now but will probably change with rebasing)
I noted that SDL_GetWindowSize and SDL_GetRendererOutputSize returned the same results for me (even with
Window's automatic scaling for non-high-DPI-enabled apps disabled) but that the SDL documentation stated the
former returned screen coordinates and the latter pixels. In that same commit I changed the dimension functions
to use SDL_GetWindowSize. I've now reversed that decision and gone back to using SDL_GetRendererOutputSize so
I can guarantee output in pixels. If use_pixels is false, the code will return coordinates in 96 DPI, so I need
to have pixel measurements for the calculations.
Again, though, I do not know if SDL_GetWindowSize returns a different value that pixel size (which it's said
to do) on macOS or iOS. I'll need to do some testing. It's possible on those platforms I won't need the 96 DPI
measurements, but it's also possible it will be needed on on platforms, since all of our code relies on pixel
measurements.
Copying the blit_helper objects when adding them to the drawing buffer was expensive since
blit_helper contains a vector of surfaces. We definitely want to copy that as little as
possible.
Also used std::move when fetching fog/shroud images. This function apparently has little
overhead, but still nice to not make copies here.
Also made get_terrain_images return void (this is just minor cleanup, not an optimization).
No need for it to return a reference to a class member.
* The vector of surfaces is now a class member variable instead of a local
variable. This saves a memory allocation every time the function is called
- which is worth it in this case, as the function is a major performance
bottleneck.
* The surfaces are now being moved instead of copied where possible. Turns
out that freeing a SDL surface is fairly expensive in performance-critical
code.
* Pointers to ToDs are now cached, reducing the number of calls to
get_time_of_day() from 37 to 7.
In a stress-test in Aetheryn's Mod at 50 % zoom, the FPS I was getting on
my PC (Intel Core i5-4430) increased from 16 to 23.
* display::w() and display::h() now simply forward to CVideo::getx() and gety() instead of querying
the underlying window object. This is a slight semantic change, since now these functions return
screen coordinates instead of pixels, but that's probably what we want in the long term anyway.
* sdl:🪟:get_flags() now returns the proper uint32_t instead of int
* Added a dedicated CVideo::window_has_flags function since testing window flags was the most common
use of the underlying sdl window outside the class.
* Made CVideo::set_window_mode private. This is an implementation detail and shouldn't be public.
* Removed BPP argument from CVideo::make_test_fake. It's never passed anything other than 32, and
we're long past the time when we care about window BPP.
* Removed sdl/window.hpp include from video.hpp. This was mostly only there to allow other area of
the code to call get_window()->get_flags() (and for display.hpp). There's no real reason for it to
be there anymore and was just making any modification to the sdl window header take ages.
Essentially, we had CVideo arguments being passed down this chain:
- game_launcher
- free-standing MP initialization functions
- campaign_controller
- playsingle_controller/playmp_controller
- play_controller
- game_display
- display
And likewise down through
- game_launcher
- editor_controller
- editor_display
- display
With only a minimal number of actual calls along the way. :| There were maybe... two remaining?
This removes the CVideo arguments and class members from both chains (except of course, game_launcher.
That's where the "real" CVideo object lives).
The display class now initializes its CVideo reference from the singleton, which is also used in the
very few other places it's needed. I also replaced a check for a null video ptr in show_tooltip()
with a faked() check (see src/tooltips.cpp). That seems to make more sense, since CVideo is never
null now.
Follow up 'ceba081542a4'. It is nice to remove the previously
announced message when announces are being delivered very quickly, but
maybe movement feedback announces should be exempt of that. Before
this rev, Whenever an 'Enemy unit sighted' message was being ordered
coupled with a subsequent 'press $HOTKEY to keep moving', the 'Enemy
unit sighted' message was getting discarded.
src/actions/move.cpp: Movement feedback is important, do not remove
previous messaging when announcing.
src/display.cpp: Do not remove previously announced label when so
requested.
src/display.hpp: Add a `struct` device meant to pass optional
arguments to `void announce(const std::string&, const color_t&, ...)`
instead of primitive typed optional arguments (one, `int`, was being
in use, I would have needed to add a second one, `bool`, but when
trying to do that, the `bool` value would be received by the function
as the `int` argument when not providing an explicit value for the
`int` argument (see `src/actions/move.cpp`). Given C++11, for optional
arguments, does not (to the extent of my understanding) allow
specifying the argument name on the calling place, I was forced into
adding this struct in order to jail all primitive typed optional
arguments.
src/synced_commands.cpp: Adapt to new public API in `class display`.
This halves the CPU usage cost of animations, which is useful especially in
water-heavy scenarios such as Dead Water.
I was worried about the possibility of regressions, but I didn't find any
in my playtesting: moved and killed units disappear from their hexes like
they should.
Awhile back I added some code to remove any resolutions from the list that exceeded the current DPI.
I seem to have misunderstood some of the functionality.
First, off, GetCurrentDisplayMode doesn't seem to return current resolution. From my tests, it seems
to return a "maximum maximized size" of some sort equal to GetUsableDisplayBounds - 1 (see below):
* Render output size: 800, 600
* Display mode size: 1536, 864
* Window size: 800, 600
* Display Bounds: x: 0, y: 0, w: 1537, h: 865
* Usable display bounds: x: 0, y: 0, w: 1537, h: 865
The actual
window size, which @celticminstrel informs me is what we should be measuring here, is actually returned
by either GetWindowSize or GetRenderOutputSize. According to SDL, the latter should return pixel size
and the former screen coordinates. In my tests, though, the results are the same. This might be different
on macOS or iOS. Either way, I've changed current_resolution(), getx(), and gety() to use the results of
GetWindowSize().
Additionally, it seems I don't need to multiply any display modes by the DPI scale factor if I check the
sizes against the aforementioned "max maximized area" w/h. For that I use GetDisplayBounds however...
though again, I'm not sure that's the best way to do this. It does seem to work correctly to fix the
aforementioned bug, anyway. I'll need to figure out more about the handling of DPI on Windows vs macOS
or iOS. There's an implication that the measurements some of these functions return is different.
The FPS cap, originally implemented in 2007, is very poorly done. It
doesn't take frame time variance into account, and is therefore almost
guaranteed to cause missed frames all the time. It doesn't increase timer
granularity on Windows, which causes SDL_Delay() to often take much longer
than intended. And it's hardcoded for 50 FPS, which fits poorly with 60 Hz
displays.
This new implementation fixes all those issues.
My experience is that the game feels much, much smoother with the new
implementation, perfectly competitive with 1.12. In my opinion, performance
is now at an acceptable level for a stable release.
I reverted the aforementioned commit after being informed it would be inferior, performance-wise.
However, this should solve that as the map is no longer constantly recreated.
This also addresses some issues I had with the original commit where I had to make some member
functions non-const.
This only includes cases where this can be done without triggering warnings about narrowing conversion.
Also includes cleanups of sdl/rect.hpp includes.
Turns out I mistook @celticminstrel's opinion that we should use include guards over pragma (737916e).
Since all major compilers support `#pragma once`, there's no reason not to use it.
For future mergability reasons, this excludes src/spirit_po and src/xBRZ. It also excludes src/boost-patched.
These were rendered unnecessary by 781276709d, which changed border rendering to use the terrain engine.
This also removes display::draw_border, which used the keys.
Not exactly sure what it meant by "sizing the vector" - the enum isn't a vector, or used in a vector. It doesn't
seem to have been relevant to max_layer_group either, since that was calculated with a -2 offset to ignore it.
Likely it had to do with the loop in drawing_buffer_key ctor, which was designed to find the group (which is
drawing_layer an enum member) "under" (ie, that contained) the specified layer. Having a dummy buffer meant the
precondition of layer < max layer (ie, the buffer) layer was always true, meaning the index was always decremented
at least once.
This fixes an issue where the zoom index wasn't properly reset when set_default_zoom was set. It also ensures
this function will only work with the set zoom levels.
This slider hasn't worked in ages, and we're planning to remove it anyway.
The slider "groove" image remains since it's hard drawn in the background image file.
Since it represents a colour delta rather than an absolute colour, the
color_t struct is not a sufficient substitute.
This partially reverts commit 0adeea43e0.
The formatting changes from that commit were not reverted, nor was
including colour in time-of-day equality. The unused tod_color::operator*=
was not restored, either.
