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

Jump to bottom

Codechanges to SDL2 driver to increase readability (and make it more like Win32 driver) #8605

Merged
merged 11 commits into from Jan 30, 2021
Merged

Codechanges to SDL2 driver to increase readability (and make it more like Win32 driver) #8605

merged 11 commits into from Jan 30, 2021

Conversation

TrueBrain
Copy link
Member

Motivation / Problem

While working on adding OpenGL support to SDL2, I found various of things in the SDL2 code that were either hard to read, made the control-flow difficult to understand, or was just a pile of "lets add this and this and this and this" in a single function.

Additionally, for OpenGL we plan to unify the naming and flow of Win32 and SDL2 driver a bit more, so when you read one or the other, you have better grasp of what is going on.

Description

This PR should make absolute zero change to what SDL2 is actually doing. That is, I did my best to not make any real change, but only move code, make it more readable, make it more like Win32, etc. If there is any change, this is most likely not intended.

Few exceptions (as always):

  • SDL2 was the only driver without fallback resolutions. This might be "strictly correct" but makes the code very special in contrast to other drivers. In fact, after OpenGL merges, I hope we can unify the drivers a bit more, as they all have near identical resolution detection code.
  • The way SDL2 did Palette animation was just .. weird. It redraws the screen twice, it follows a very odd flow (especially when looking at other drivers). So I took some freedom to rework how the flow is; it is now very similar to Win32, doesn't do unneeded redraws, and only has a single huge-blob-of-comments about the RGB surface used.

Tip for reviewer: review this commit by commit, they are all fixing a single problem, and they do not depend on each other. It could as well have been 11 separate PRs.

Limitations

  • I tested this on Windows, where we strictly seen don't support SDL on. This needs testing on Linux by people to confirm my statement: this makes 0 change, holds.
  • SDL1 driver has similar issues. I don't mind backporting this to SDL1 too, but before I do I would rather know if we are going to keep SDL1 driver in, or if we are going to drop it soon (tm).

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

michicc
michicc reviewed Jan 30, 2021
View reviewed changes
src/video/sdl2_v.cpp Outdated Show resolved Hide resolved
@TrueBrain
Codechange: [SDL2] remove include-protection
78b15f3 
This is already done by CMake: if SDL2 is not detected, this file
is not included.
@TrueBrain
Codechange: [SDL2] reworked FindResolutions to be more like the rest
37b553a 
There was no default resolution fallback, and the code was different
from the win32 driver. It is now named the same and much more
similar.
@TrueBrain
Codechange: [SDL2] Move FindStartupDisplay to its own function
d25f169
@TrueBrain
Codechange: [SDL2] Only set _cur_palette, never _local_palette
556b7f4
@TrueBrain
Codechange: [SDL2] Minor code cleanup to remove silly variable
96f2d80
@TrueBrain
Codechange: [SDL2] Only prepare "caption" if you are going to us it
c755153
@TrueBrain
Codechange: [SDL2] reworked the different surfaces to make it more re…
1af8e2e 
…adable
@TrueBrain
Codechange: [SDL2] Split away CreateMainWindow from CreateMainSurface
fec0902 
This makes the code a bit more readable, as both intentions are
more clear, and there is less nesting in the main function.
@TrueBrain
Fix: [SDL2] Display why SDL_CreateWindow() failed in case it does
cb45fec 
All SDL_NNN errors print SDL_GetError, except for this one place.
@TrueBrain
Codechange: [SDL2] Rework how palette is updated
bf2c13c 
It now follows more what the Win32 driver does, and has far less
exceptions and special casing.

MakePalette creates the Palette and prepares surface.
UpdatePalette updates the Palette.
CheckPaletteAnim checks if UpdatePalette needs to be called and
  marks the whole screen dirty so DrawSurfaceToScreen will do a
  full redraw.
@TrueBrain
Codechange: [SDL2] Use MakeDirty() to force a redraw
68a72a9 
The original code is "strictly correct", but just reads really
weird, and we use MakeDirty() in several other places instead too.
michicc
michicc approved these changes Jan 30, 2021
View reviewed changes
Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

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

Might be fine 🍤

@TrueBrain TrueBrain merged commit 0e54c32 into OpenTTD:master Jan 30, 2021
@TrueBrain TrueBrain deleted the sdl-cleanup branch January 30, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michicc michicc michicc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@TrueBrain @michicc