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

Change #6173: Update SDL driver to use SDL 2.0 #7086

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@nikolas
Copy link
Contributor

commented Jan 23, 2019

This has been working well for me. Based on the documentation here: https://wiki.libsdl.org/MigrationGuide

Will require testing on different setups.

Here's the FPS window on both old and new SDL, rendering a whole map.

SDL 1.2:
2019-01-22-213340_1366x768_scrot

SDL 2.0:
2019-01-22-214647_1366x768_scrot

closes #7296

@nikolas nikolas force-pushed the nikolas:sdl2 branch from 41c3401 to 1ffd64d Jan 23, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

The number to pay attention to on those screenshots: Video output has decreased from 4 ms to 2 ms with the SDL 2 driver.
Not tested it myself.

@nikolas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

Also worth mentioning is that this changes the SDL driver from software rendering to hardware rendering, which has to potential to cause problems since we're now relying on the user's graphics drivers.

I've seen this branch segfault when the program starts on an old Mac mini I have, running Linux and the nouveau DRI drivers. This computer can run OpenTTD with SDL 1.2 just fine, and obviously we don't want to introduce this regression.

So I'll need to implement a software rendering fallback using either SDL_CreateSoftwareRenderer() or SDL_CreateRenderer() with the SDL_RENDERER_SOFTWARE flag. I'm not sure exactly how to "fall back" to the software renderer, but I'm sure there's some examples around. I'll take a look.

@nikolas nikolas force-pushed the nikolas:sdl2 branch from 1ffd64d to 0a9d162 Jan 23, 2019
@LordAro LordAro added the wip label Jan 24, 2019
@nikolas nikolas force-pushed the nikolas:sdl2 branch 8 times, most recently from 4496b98 to 0a60edb Jan 24, 2019
@nikolas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

Alright, I've gotten a chance to debug the issue on the Mac mini system I mentioned above. It's working fine with an update I just made to this branch: using a renderer created with the SDL_RENDERER_SOFTWARE flag instead of SDL_RENDERER_ACCELERATED. Unfortunately, just passing in 0 here and letting SDL choose doesn't fix the problem: seems to be a driver bug, which other people have run into with these nouveau drivers.

Anyways, we're not losing anything by using software rendering here, as that's how it currently works. Please test this branch, anyone who's able and willing, and let me know of any problems you see!

@glx22

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

Of course CI fails for linux because SDL2-dev is not installed

@nikolas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

Hmm, I can't see the exact libsdl failure - though I'm not used to Azure.

I see a bunch of messages like "Unable to find image 'openttd/compile-farm-ci:linux-amd64-gcc-6' locally" here: https://github.com/OpenTTD/OpenTTD/pull/7086/checks?check_run_id=55198664

Anyways, this Dockerfile would need to be updated to include libsdl2-dev. I'll make a PR.
https://github.com/OpenTTD/CompileFarm/blob/master/base-linux/Dockerfile#L29

@glx22

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

nikolas added a commit to nikolas/CompileFarm that referenced this pull request Jan 27, 2019
@TrueBrain

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

If you go to the Azure results, here: https://dev.azure.com/openttd/OpenTTD/_build/results?buildId=607
You can click a build that failed. Then you can click again on the step that failed. There you see: 2019-01-26T23:49:22.1701030Z configure: error: no video driver development files found. We have an open bug to make the reporting more verbose.

@nikolas nikolas force-pushed the nikolas:sdl2 branch 7 times, most recently from c3493e4 to 07dd81c Jan 27, 2019
@nikolas nikolas force-pushed the nikolas:sdl2 branch 2 times, most recently from ea749d0 to cf22438 Feb 5, 2019
src/video/sdl_v.cpp Outdated Show resolved Hide resolved
@nikolas

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

It's possible that ShouldUseTextureFramebuffer() is returning true for these nvidia situations, causing UpdateWindowFrameBuffer() to call SDL_UpdateWindowTexture(), as seen in the code linked to above. ShouldUseTextureFramebuffer() offers a few paths out of this, seen here: https://hg.libsdl.org/SDL/file/05dddfb66b85/src/video/SDL_video.c#l166

I've added a new commit to this branch that should tell SDL that we don't want any hardware acceleration, and in turn, no texture updating. We only want to be dealing with SDL surfaces, not textures, in this SDL 2 driver at the moment.

Anyways.. this is just kind of blind debugging. We'll see if this makes any difference for @LordAro or @stormcone.

@stormcone

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

@nikolas Probably you are right. I tried out your branch with a Fedora live DVD, which using the Nouveau driver, and I was able to start a new game with SDL2.

By the way is it possible to that the sdl2_v.cpp use "SDL2" in the debug messages instead of "SDL"?
For example instead of:
DEBUG(driver, 1, "SDL: using driver '%s'", dname);
-->
DEBUG(driver, 1, "SDL2: using driver '%s'", dname);

@nikolas

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@stormcone sure, I just made that change.

@nikolas

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@stormcone Good to hear it works with the Nouveau driver, but let me know when you're able to test this with the Nvidia proprietary driver again.

@stormcone

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

@nikolas Thank you. :)

I tried out your new commit with the disabled hardware acceleration and now the game starts normally. I can use the menu and start a new game. So looks like that commit solves the problem. :)

Edit: I mean with the NVIDIA driver.

@LordAro

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Confirmed, now runs

@LordAro

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Simulation rate is still consistently running at 1.02x (for me) but that's the same as SDL1, so I'm not too concerned about it

@nikolas nikolas force-pushed the nikolas:sdl2 branch 2 times, most recently from d535ec5 to 8306506 Jul 26, 2019
@Nik-mmzd

This comment has been minimized.

Copy link

commented Jul 28, 2019

I'm on nikolas/OpenTTD@8306506; linux; sdl 2.0.9
Found two bugs:

  1. Numpad Enter does not acts like Enter
  2. Arrow buttons do not move cursor but ctrl-right/left arrow moves cursor by a word

Also, thanks for fixed #7296

@Nik-mmzd

This comment has been minimized.

Copy link

commented Jul 29, 2019

Wow, it works, thanks

@nikolas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@nielsmh this is ready for another review, since known issues are now fixed. Just fyi.

@LordAro LordAro requested a review from nielsmh Aug 2, 2019
@nikolas nikolas force-pushed the nikolas:sdl2 branch from f6c7c83 to 5054aeb Aug 2, 2019
Copy link
Member

left a comment

Nearly there, I think :)

src/os/unix/unix.cpp Outdated Show resolved Hide resolved
src/video/sdl2_v.cpp Outdated Show resolved Hide resolved
@nikolas nikolas force-pushed the nikolas:sdl2 branch from 5054aeb to e2be5e9 Aug 17, 2019
@nikolas nikolas force-pushed the nikolas:sdl2 branch from e2be5e9 to 135c792 Sep 9, 2019
Copy link
Member

left a comment

👍

@nikolas nikolas force-pushed the nikolas:sdl2 branch from 135c792 to 4c683a6 Sep 14, 2019
@Milek7 Milek7 referenced this pull request Sep 17, 2019
@nikolas nikolas force-pushed the nikolas:sdl2 branch 2 times, most recently from 6f21eea to e69eb4e Sep 17, 2019
Copy link
Member

left a comment

Minor, but it triggered my OCD :P

src/crashlog.cpp Outdated Show resolved Hide resolved
src/crashlog.cpp Outdated Show resolved Hide resolved
@nikolas nikolas force-pushed the nikolas:sdl2 branch from e69eb4e to 0948422 Sep 17, 2019
@nikolas nikolas force-pushed the nikolas:sdl2 branch from 0948422 to 6a833f5 Sep 17, 2019
@nielsmh nielsmh merged commit 2d27e8e into OpenTTD:master Sep 19, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190917.6 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.