Page MenuHomeWildfire Games

Report SDL2 version and video backend in hwdetect
ClosedPublic

Authored by linkmauve on Dec 19 2019, 4:08 PM.

Details

Summary

This is helpful to know which backend the user is using, as well as version specific issues.

Test Plan
  • Run pyrogenesis.
  • Inspect ~/.config/0ad/logs/userreport_hwdetect.txt
  • It should contain two new fields, sdl2_version and sdl2_video_backend.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Fix missing include on Windows.
  • Fix build with SDL2 2.0.3 (used in the CI).

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/792/display/redirect

You need to update report version too.

source/ps/GameSetup/HWDetect.cpp
46 ↗(On Diff #10642)

It's not usual include, should be in the system "section". Why it's not included in libsdl.h?

372 ↗(On Diff #10642)

I think just SDL, as most of SDL API doesn't contain 2 in names.

375 ↗(On Diff #10642)

It returns compiled version: https://wiki.libsdl.org/SDL_version.

378 ↗(On Diff #10642)

We use the secure function instead sprintf_s.

382 ↗(On Diff #10642)

Early return instead and use the return value in-place.

384 ↗(On Diff #10642)

I'd like to use std::string here if possible, as we don't have frequent allocations here.

387 ↗(On Diff #10642)

case should have the same indentation as the switch: https://trac.wildfiregames.com/wiki/Coding_Conventions.

410 ↗(On Diff #10642)

We need to avoid default in switch to not miss a new value, also not all values are reported.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1308/display/redirect

linkmauve marked 6 inline comments as done.Dec 19 2019, 4:41 PM
linkmauve added inline comments.
source/ps/GameSetup/HWDetect.cpp
46 ↗(On Diff #10642)

What do you mean by system section?

410 ↗(On Diff #10642)

This enum is definitely open-ended, with new backends being added in new SDL2 versions, it would break the build on newer releases to not include that default.

linkmauve updated this revision to Diff 10643.Dec 19 2019, 4:42 PM

Fix all comments by Vladislav.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/793/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1309/display/redirect

vladislavbelov added inline comments.Dec 19 2019, 4:51 PM
source/ps/GameSetup/HWDetect.cpp
46 ↗(On Diff #10642)
410 ↗(On Diff #10642)

That's why we can have #if :) We fix the version where we know all values and add default for upper:

#if SDL_VERSION_GREATER(2, 0, 10)
	default:
		break;
#endif
Stan added a subscriber: Stan.Dec 19 2019, 4:59 PM
source/ps/GameSetup/HWDetect.cpp
410 ↗(On Diff #10642)

What about LOGWARNING("")? It would be nice to actually have errors when they occur? :)

linkmauve updated this revision to Diff 10644.Dec 19 2019, 5:08 PM
  • Put the include in the system section.
  • Add a warning for SDL 2.0.11+ new platforms.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1310/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/794/display/redirect

linkmauve marked 4 inline comments as done.Dec 19 2019, 5:10 PM
linkmauve added inline comments.
source/ps/GameSetup/HWDetect.cpp
46 ↗(On Diff #10642)

It isn’t included as part of libsdl.h because it includes a bunch of platform-specific headers we might not want on all platforms, which may pollute the global environment.

linkmauve updated this revision to Diff 10645.Dec 19 2019, 5:12 PM

Fix wrong introduction version for Vivante, thanks Stan` for spotting that!

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1311/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/795/display/redirect

source/ps/GameSetup/HWDetect.cpp
359 ↗(On Diff #10645)

It needs to update the report version.

380 ↗(On Diff #10645)

Use ARRAY_SIZE(version) instead of 16.

384 ↗(On Diff #10645)

Use ARRAY_SIZE(version) instead of 16.

389 ↗(On Diff #10645)

You might want to print error in that case (SDL_GetError()).

linkmauve updated this revision to Diff 10781.Wed, Dec 25, 12:14 PM
  • Use ARRAY_SIZE() instead of hardcoding the array size.
  • Use the correct first version for WinRT and OS/2.
  • Increment the reportVersion.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/885/display/redirect

linkmauve marked 4 inline comments as done.Wed, Dec 25, 12:20 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1401/display/redirect

This only depends on D2499’s removal of WGL now.

It seems that it doesn't compile because SDL_syswm.h includes windows.h with extern "C", but previous ones include without. My suggestion is to move SDL2 information functions to libraries and include only SDL related things. It's also good because you won't have SDL2 internal logic in HWDetect.

source/ps/GameSetup/HWDetect.cpp
390 ↗(On Diff #10781)

I mean to print the real error, it's available from SDL_GetError().

linkmauve updated this revision to Diff 10782.Wed, Dec 25, 1:08 PM

Actually use SDL_GetError().

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/886/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1402/display/redirect

Won’t that be fixed once we don’t redefine the WGL types in wgl.h (that is, once that file gets nuked)?

Won’t that be fixed once we don’t redefine the WGL types in wgl.h (that is, once that file gets nuked)?

Error - yes, warning - no.

linkmauve updated this revision to Diff 11095.Sat, Jan 18, 3:34 PM

Move the implementation in lib/external_libraries/libsdl.cpp

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1604/display/redirect

linkmauve updated this revision to Diff 11096.Sat, Jan 18, 3:39 PM

Fix HWDetect.cpp.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1086/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1087/display/redirect

linkmauve updated this revision to Diff 11097.Sat, Jan 18, 3:46 PM

Include "pregenerated.h"

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/external_libraries/libsdl.h
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2015"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1605/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1088/display/redirect

linkmauve updated this revision to Diff 11098.Sat, Jan 18, 4:05 PM

Fix the copyright year.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1089/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/external_libraries/libsdl.h
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2015"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1606/display/redirect

vladislavbelov added inline comments.Sat, Jan 18, 4:13 PM
source/lib/external_libraries/libsdl.cpp
19 ↗(On Diff #11098)

You need to include libsdl.h as well:

#include "precompiled.h"

#include "libsdl.h"

#include <SDL_syswm.h>
34 ↗(On Diff #11098)

Maybe to avoid possible warning and undefined function results use a variable. Like:

const char* subsystem = "unknown";
switch (wminfo.subsystem)
{
case SDL_SYSWM_WAYLAND:
	subsystem = "Wayland";
	break;
case SDL_SYSWM_X11:
}
return subsystem;

Or just add return "unknown"; to the end of function?

source/ps/GameSetup/HWDetect.cpp
380 ↗(On Diff #11098)

"sdl_*" here and below.

linkmauve added inline comments.Sat, Jan 18, 4:19 PM
source/lib/external_libraries/libsdl.cpp
34 ↗(On Diff #11098)

There shouldn’t have been any warning or undefined result, because of the default you made me make conditional on line 66.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/182/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1607/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/183/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/184/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/185/display/redirect

linkmauve updated this revision to Diff 11101.Sat, Jan 18, 4:36 PM
  • Rename the sdl2_* keys to sdl_*.
  • Include used CLogger.h.
  • Include useless libsdl.h but that’s part of the project guidelines.
  • Attempt a workaround for MSVC2015.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1090/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1608/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/186/display/redirect

vladislavbelov accepted this revision.Sat, Jan 18, 5:45 PM

I've tested the patch on Windows VS2015, it works and looks good for me.

This revision is now accepted and ready to land.Sat, Jan 18, 5:45 PM