Page MenuHomeWildfire Games
Differential D4493

Moves vertex and index buffer management to CDeviceCommandContext
ClosedPublic
Actions

Authored by vladislavbelov on Feb 16 2022, 7:54 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP26406: Moves GL vertex and index buffer management to CDeviceCommandContext.
Summary

First step to abstract vertex attributes manipulations. Also we need to rename/refactor VertexArray/VertexBuffer as they're used not only for vertices but also for indices.

Test Plan
  1. Apply the patch and compile the game
  2. Make sure the game works in Release and Debug modes for GL and GL ARB
  3. Compare performance for some complex scene
  4. Check Atlas and terrain manipulations (like painting and sculpting)

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

vladislavbelov created this revision.Feb 16 2022, 7:54 PM
Vulcan added a comment.Feb 16 2022, 8:06 PM

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

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

Vulcan added a comment.Feb 16 2022, 8:17 PM

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

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

Vulcan added a comment.Feb 16 2022, 8:34 PM

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

builderr-release-gcc7.txt
In file included from ../../../source/pch/atlas/precompiled.h:26:
../../../source/tools/atlas/GameInterface/Messages.h: In function 'void AtlasMessage::fGetTerrainGroupPreviews(AtlasMessage::qGetTerrainGroupPreviews*)':
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]

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

Harbormaster completed remote builds in B19511: Diff 19691.Feb 16 2022, 8:34 PM
vladislavbelov requested review of this revision.Feb 16 2022, 8:34 PM
Stan added a subscriber: Stan.Feb 17 2022, 12:47 AM
Stan added inline comments.
source/renderer/VertexBuffer.h
160 ↗(On Diff #19691)

memory

source/renderer/backend/gl/DeviceCommandContext.cpp
116 ↗(On Diff #19691)

Nullptr?

130 ↗(On Diff #19691)

Should it be logmessage?

vladislavbelov marked 3 inline comments as done.Feb 17 2022, 8:44 AM
vladislavbelov added inline comments.
source/renderer/backend/gl/DeviceCommandContext.cpp
130 ↗(On Diff #19691)

No, it might happen during window resizing for some platforms. I don't think we need warning on each resize.

Stan added inline comments.Feb 17 2022, 8:47 AM
source/renderer/backend/gl/DeviceCommandContext.cpp
130 ↗(On Diff #19691)

Logmessage only shows up in the mainlog.
You wouldn't see it :)
There is logmessagerender for that purpose iirc.

vladislavbelov marked 2 inline comments as done.Feb 17 2022, 12:33 PM
vladislavbelov added inline comments.
source/renderer/backend/gl/DeviceCommandContext.cpp
130 ↗(On Diff #19691)

Ah, ok, that makes sense.

vladislavbelov updated this revision to Diff 19695.Feb 17 2022, 12:41 PM
vladislavbelov marked an inline comment as done.
Vulcan added a comment.Feb 17 2022, 12:52 PM

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

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

Vulcan added a comment.Feb 17 2022, 12:57 PM

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

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

Vulcan added a comment.Feb 17 2022, 1:13 PM

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

builderr-release-gcc7.txt
In file included from ../../../source/pch/atlas/precompiled.h:26:
../../../source/tools/atlas/GameInterface/Messages.h: In function 'void AtlasMessage::fGetTerrainGroupPreviews(AtlasMessage::qGetTerrainGroupPreviews*)':
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]

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

Harbormaster completed remote builds in B19515: Diff 19695.Feb 17 2022, 1:13 PM
vladislavbelov updated this revision to Diff 19696.Feb 17 2022, 4:48 PM
Vulcan added a comment.Feb 17 2022, 5:00 PM

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

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

Vulcan added a comment.Feb 17 2022, 5:04 PM

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

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

Vulcan added a comment.Feb 17 2022, 5:20 PM

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

builderr-release-gcc7.txt
In file included from ../../../source/pch/atlas/precompiled.h:26:
../../../source/tools/atlas/GameInterface/Messages.h: In function 'void AtlasMessage::fGetTerrainGroupPreviews(AtlasMessage::qGetTerrainGroupPreviews*)':
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:305:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]

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

Harbormaster completed remote builds in B19516: Diff 19696.Feb 17 2022, 5:20 PM
Stan added a comment.Feb 18 2022, 5:16 PM

Seems to work for me.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 18 2022, 6:33 PM
This revision was landed with ongoing or failed builds.
Closed by commit rP26406: Moves GL vertex and index buffer management to CDeviceCommandContext. (authored by vladislavbelov). · Explain Why
This revision was automatically updated to reflect the committed changes.
vladislavbelov added a commit: rP26406: Moves GL vertex and index buffer management to CDeviceCommandContext..

Revision Contents
Changeset List

Diff 19706

View Options

ps/trunk/source/graphics/MiniMapTexture.cpp

Loading...
View Options

ps/trunk/source/graphics/ParticleEmitter.h

Loading...
View Options

ps/trunk/source/graphics/ParticleEmitter.cpp

Loading...
View Options

ps/trunk/source/renderer/DecalRData.cpp

Loading...
View Options

ps/trunk/source/renderer/HWLightingModelRenderer.h

Loading...
View Options

ps/trunk/source/renderer/HWLightingModelRenderer.cpp

Loading...
View Options

ps/trunk/source/renderer/InstancingModelRenderer.h

Loading...
View Options

ps/trunk/source/renderer/InstancingModelRenderer.cpp

Loading...
View Options

ps/trunk/source/renderer/ModelRenderer.cpp

Loading...
View Options

ps/trunk/source/renderer/ModelVertexRenderer.h

Loading...
View Options

ps/trunk/source/renderer/OverlayRenderer.cpp

Loading...
View Options

ps/trunk/source/renderer/ParticleRenderer.cpp

Loading...
View Options

ps/trunk/source/renderer/PatchRData.h

Loading...
View Options

ps/trunk/source/renderer/PatchRData.cpp

Loading...
View Options

ps/trunk/source/renderer/TerrainRenderer.cpp

Loading...
View Options

ps/trunk/source/renderer/TexturedLineRData.cpp

Loading...
View Options

ps/trunk/source/renderer/VertexArray.h

Loading...
View Options

ps/trunk/source/renderer/VertexArray.cpp

Loading...
View Options

ps/trunk/source/renderer/VertexBuffer.h

Loading...
View Options

ps/trunk/source/renderer/VertexBuffer.cpp

Loading...
View Options

ps/trunk/source/renderer/VertexBufferManager.h

Loading...
View Options

ps/trunk/source/renderer/VertexBufferManager.cpp

Loading...
View Options

ps/trunk/source/renderer/WaterManager.cpp

Loading...
View Options

ps/trunk/source/renderer/backend/gl/Buffer.h

Loading...
View Options

ps/trunk/source/renderer/backend/gl/Buffer.cpp

Loading...
View Options

ps/trunk/source/renderer/backend/gl/Device.h

Loading...
View Options

ps/trunk/source/renderer/backend/gl/Device.cpp

Loading...
View Options

ps/trunk/source/renderer/backend/gl/DeviceCommandContext.h

Loading...
View Options

ps/trunk/source/renderer/backend/gl/DeviceCommandContext.cpp

Loading...
Wildfire Games · Phabricator