Skip to content
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

reorder vissprites, so that objects with higher map indices appear in front #1165

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

fabiangreffrath
Copy link
Owner

@fabiangreffrath fabiangreffrath commented Aug 7, 2023

No description provided.

@fabiangreffrath fabiangreffrath merged commit 7202ffa into master Aug 7, 2023
6 checks passed
@fabiangreffrath fabiangreffrath deleted the stable_msort branch August 7, 2023 16:21
@kraflab
Copy link
Collaborator

kraflab commented Aug 7, 2023

What was the issue that this fixed?

@fabiangreffrath
Copy link
Owner Author

fabiangreffrath commented Aug 7, 2023

Vanilla Doom sorts vissprites for objects that are placed on the exact same spot on the map (and thus share the same scale value) using a stable sort algorithm. That algorithm preserves the order of the objects' "map indices", so that vissprites for objects that are added later to a map are drawn in front of the others. However, starting with Boom, the sorting was changed. It was still stable sorting in MBF, but the order was reversed. This patch fixes it back to the Vanilla behavior.

There is a nice thread about this in the ZDoom forums:

https://forum.zdoom.org/viewtopic.php?f=2&t=24375

@fabiangreffrath
Copy link
Owner Author

PrBoom has fixed this in the same manner as Zdoom by reversing the order in which the visplanes are added up. Is just hidden behind some rather obscure config variable. @rfomin I'm considering to do the same instead of changing the sorting algorithm itself? And I'm still considering to do this based on complevel.

@kraflab
Copy link
Collaborator

kraflab commented Aug 7, 2023

Thanks for the info 👍

@rfomin
Copy link
Collaborator

rfomin commented Aug 8, 2023

PrBoom has fixed this in the same manner as Zdoom by reversing the order in which the visplanes are added up.

But sprite flickering is not fixed in PrBoom+/DSDA-Doom. Test WAD: sort.zip (with software render, fire the gun, the order of the sprites will change).

Is just hidden behind some rather obscure config variable. And I'm still considering to do this based on complevel.

The config variable is sprites_doom_order, which is enabled by default in PrBoom+ and removed in DSDA-Doom. So ZDoom and PrBoom+/DSDA-Doom are fixed by default, I think we don't need complevel or option for that.

I'm considering to do the same instead of changing the sorting algorithm itself?

I think we should fix sorting algorithm. Merge Sort should be stable by design. From Wikipedia:

Most implementations produce a stable sort

JNechaevsky added a commit to JNechaevsky/international-doom that referenced this pull request Aug 8, 2023
@fabiangreffrath
Copy link
Owner Author

DSDA will need this fix to preserve stable sorting in Vanilla order:

--- a/prboom2/src/r_things.c
+++ b/prboom2/src/r_things.c
@@ -1324,7 +1324,7 @@ static void msort(vissprite_t **s, vissprite_t **t, int n)
       msort(s1, t, n1);
       msort(s2, t, n2);
 
-      while ((*s1)->scale > (*s2)->scale ?
+      while ((*s1)->scale >= (*s2)->scale ?
              (*d++ = *s1++, --n1) : (*d++ = *s2++, --n2));
 
       if (n2)

@rfomin
Copy link
Collaborator

rfomin commented Aug 8, 2023

DSDA will need this fix to preserve stable sorting in Vanilla order:

Maybe we should implement it the same way in Woof? This fix seems more logical to me. Performance looks the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants