Unhardcode vertices#21059
Conversation
PunkPun
force-pushed
the
vertex
branch
4 times, most recently
from
9baf2a8 to
a65b158
Compare
RoosterDragon
left a comment
RoosterDragon
left a comment
There was a problem hiding this comment.
General idea looks solid to me, some tidy-up comments added.
PunkPun
force-pushed
the
vertex
branch
2 times, most recently
from
bf1f780 to
1d27ee5
Compare
anvilvapre
left a comment
anvilvapre
left a comment
There was a problem hiding this comment.
Seems clean. Untested. I have limited knowledge of shaders.
| Stack<T[]> pool; | ||
| if (!vertexBufferPools.TryGetValue(typeof(T), out var poolObject)) | ||
| { | ||
| pool = new Stack<T[]>(); |
There was a problem hiding this comment.
Allocation while holding lock. May take long.
There was a problem hiding this comment.
this should be called only once per type
| lock (verticesPool) | ||
| if (size <= BatchSize && verticesPool.Count > 0) | ||
| return verticesPool.Pop(); | ||
| lock (vertexBufferPools) |
There was a problem hiding this comment.
How many types do you expect there to be.
(Perhaps) better to have the lock at poolObject level.
There was a problem hiding this comment.
I expect to have a bunch of more vertex types. At least 4
| var filename = Path.Combine(Platform.EngineDir, "glsl", name + "." + ext); | ||
| var code = File.ReadAllText(filename); | ||
|
|
||
| var version = OpenGL.Profile == GLProfile.Embedded ? "300 es" : |
There was a problem hiding this comment.
Can this move into ShaderBindings to avoid magic manipulation of shader code that implementations can't control?
There was a problem hiding this comment.
I need to change the scope of this class to OpenRA.Platforms.Default or pass the profile
There was a problem hiding this comment.
it's actually hard to do either. This is currently dependency injected into a scope that has no idea about opengl
|
|
||
| WorldSpriteRenderer = new SpriteRenderer(this, Context.CreateShader("combined")); | ||
| var combinedBindings = new CombinedShaderBindings(); | ||
| WorldSpriteRenderer = new SpriteRenderer(this, Context.CreateShader(combinedBindings)); |
There was a problem hiding this comment.
For a future PR (definitely out of scope here): Would it be possible to have a very minimal (no palettes, no secondary sheet, no tint etc) renderer hardcoded for use by the load screen, and then defer all of these ones until after ModData is available? This would then let shaders modify their behaviour (e.g. changing the shader code that is loaded) based on the manifest data.
There was a problem hiding this comment.
world renderers can be deferred without errors. The instantiation of whole classes can be deferred.
The problem comes with the UI renderer, there are some issues when trying to delay its shader. Granted I haven't fully tried to fix them
There was a problem hiding this comment.
Those problems will be due to the loadscreens right? this is why I suggested keeping a minimal one around for that.
There was a problem hiding this comment.
I've delayed the initialisation on ModelRenderer in #21066
|
fixed |
5 participants
This PR is a large step towards custom shader support. The first commit and
ShaderBindingsclass are inspired by #19519 and #20581. In theoryShaderBindingswill be absorbed into aMaterialclass in the future.Unhardcoding verticies has allowed me to remove 4 floats from a voxel vertex. While it should in theory improve perf I didn't notice any significant change on TS shellmap.