Page MenuHomeWildfire Games

Do not use GPUSkinning without glsl [Crash fix]
ClosedPublic

Authored by Silier on Nov 13 2019, 7:35 PM.

Details

Summary

VertexAttribPointer is not implemented on ARB and GLES, but is called when gpuskinning.
This patch is avoiding that to happen the way that if gpuskinning is chosen and glsl is not prefered, gpuskinning is not done.

Test Plan

Run game with gpuskinning = "true" in config file and with prefer glsl disabled.
Confirm game is not crashing with this patch.

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

Silier created this revision.Nov 13 2019, 7:35 PM
Owners added a subscriber: Restricted Owners Package.Nov 13 2019, 7:35 PM

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

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

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

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

vladislavbelov added inline comments.
source/renderer/Renderer.cpp
563 ↗(On Diff #10319)

I think it's incorrect to insert it inplace. Since we want to disable GPU skinning at all in that case. Also we don't have to do some work in Model.cpp related to GPU skinning in case it's disabled.

vladislavbelov requested changes to this revision.Jan 3 2020, 8:04 PM
This revision now requires changes to proceed.Jan 3 2020, 8:04 PM
Silier updated this revision to Diff 13564.Sep 27 2020, 4:28 PM

disable gpuskinning if glsl is disabled

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

builderr-debug-macos.txt
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
builderr-release-macos.txt
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols

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

Stan added inline comments.Sep 27 2020, 5:45 PM
source/renderer/RenderingOptions.cpp
116 ↗(On Diff #13564)

Missing a check I think

Silier added inline comments.Sep 27 2020, 6:45 PM
source/renderer/RenderingOptions.cpp
116 ↗(On Diff #13564)

yeah

Silier updated this revision to Diff 13565.Sep 27 2020, 6:47 PM

fix check

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

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols

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

vladislavbelov added inline comments.Sep 27 2020, 7:05 PM
source/renderer/RenderingOptions.cpp
116 ↗(On Diff #13565)

Braces are on new line.

142 ↗(On Diff #13565)

Braces are on new line.

144 ↗(On Diff #13565)

It disables gpu skinning until the game restart. It means you can't toggle "Prefer GLSL" and test the game w/ and w/o GPU skinning.

Stan added inline comments.Sep 27 2020, 7:58 PM
source/renderer/RenderingOptions.cpp
144 ↗(On Diff #13565)

If you want to fix it you need to recompile anyway so...

vladislavbelov added inline comments.Sep 27 2020, 8:09 PM
source/renderer/RenderingOptions.cpp
144 ↗(On Diff #13565)

No, I mean, when someone will call m_GPUSkinning = true; SetPreferGLSL(false); SetPreferGLSL(true); it (m_GPUSkinning) won't be restored.

Stan added inline comments.Sep 27 2020, 8:14 PM
source/renderer/RenderingOptions.cpp
144 ↗(On Diff #13565)

You can't set it in the game and the config isn't hotloaded is it ?

vladislavbelov added inline comments.Sep 27 2020, 8:18 PM
source/renderer/RenderingOptions.cpp
144 ↗(On Diff #13565)

It doesn't matter. Example: in config you have gpuskinning = true then you toggle "Prefer GLSL". And that disables GPU skinning until the game restart.

Stan added inline comments.Sep 27 2020, 8:25 PM
source/renderer/RenderingOptions.cpp
144 ↗(On Diff #13565)

Ah gotcha.

so you suggest to add variable to keep track if gpu skinning was disabled or to write custom getter for gpuskinning and disable it silently?

In D2423#132496, @Angen wrote:

so you suggest to add variable to keep track if gpu skinning was disabled or to write custom getter for gpuskinning and disable it silently?

May save or may reload its value from the config.

Silier updated this revision to Diff 13583.Sep 30 2020, 6:48 PM

reload gpuskinning from config when disabled and enabling preferglsl

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

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols

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

vladislavbelov accepted this revision.Oct 11 2020, 3:59 PM

The solution isn't the best, because you need to re-read the config (or store somewhere else). But usage of rendering options is bit a of mess anyway, so the patch is good enough for it.

This revision is now accepted and ready to land.Oct 11 2020, 3:59 PM
This revision was automatically updated to reflect the committed changes.