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
Added shadow rendering for resource sprites #18814
Conversation
You don't need to touch Please also only enable this if the mod explicitly sets |
1ee654f
to
2e94edc
Compare
I rewrote this. A programming mistake made me think there can be only one |
2e94edc
to
0f41f53
Compare
{ | ||
foreach (var kv in spriteLayers) | ||
{ | ||
if (palette != kv.Key) | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. The kv.Value.Clear(cell);
needs to run to make sure that the old sprite is cleared if the resource changes to something that uses a different palette.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't clear the old sprite otherwise I see only the shadows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me be clearer: this will cause corrupted states where two (four if you count shadows) sprites will be rendered on the same cell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the resource layer only allows one resource per tile so this shouldn't be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I said in my first comment, the problem occurs when the one resource in the tile changes from type A which uses palette X to type B using palette Y. Adding this check stops the kv.Value.Clear(cell);
below from clearing sprite A, which means that both sprites A and B will be rendered even though the cell no longer contains A.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes things worse:
I pushed it to https://github.com/Mailaender/OpenRA/tree/multi-sheet-resource-renderer as this pull request is in a working condition for quite some time now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fundamental problem is that the order that you loop over a dictionary is undefined, so under the old code there is no way to control which layer is drawn on top of the other. This wasn't a problem when the trait guaranteed that different layers never rendered to the same cells, but is a problem now with shadows.
#18982 removed the limitations that forced the ResourceRenderer to use a dictionary in the first place, giving it back control of the rendering order. Your branch reverts all those fixes, so its no wonder it didn't work!
627ac9e shows what I meant, which is much simpler. (EDIT: Updated to new commit with shadow and blend mode fixes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ShadowPalette
is also a problem, since we're actively removing those from all traits as part of adding support for 32bit sprites. Adapting this PR to use tint-based shadows is easy enough, but requires changes to TerrainSpriteLayer
. At this point, if you don't object, it would be easier if I take over and file a new PR for this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the Sequence-based shadow frames have never supported a separate shadow palette, so adding that here is not needed. Dropping the ShadowPalette
definition simplifies this further, and avoids the need for any other changes: 627ac9e (EDIT: Updated commit to remove redundant BlendMode check)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#18982 has been merged.
0f41f53
to
adc7196
Compare
adc7196
to
63acf46
Compare
I filed my rebased/simplified version as #19059, so closing this one as superseded. |
Closes #18780.