fabiangreffrath / crispy-doom

Crispy Doom is a limit-removing enhanced-resolution Doom source port based on Chocolate Doom.
https://fabiangreffrath.github.io/crispy-homepage
GNU General Public License v2.0
800 stars 131 forks source link

Back to the Roots #31

Closed fabiangreffrath closed 10 years ago

fabiangreffrath commented 10 years ago

I have the feeling that I have recently lost focus in the development of Crispy Doom. It was intended as "Vanilla with Vanilla-compatible enhancements". However, many Vanilla-incompatible features have been added recently and "if (singleplayer)" is scattered all over the code. I am considering to remove most of these features again and follow some kind of "feature policy":

Features should be user-visible (and drastic, but that's subjective) and must be Vanilla-compatible (with stretches, e.g. weapon sprite centering). If they are not Vanilla-compatible, they must be switchable (e.g. vertical aiming, jumping, recoil). If a feature is Vanilla-incompatible, but doesn't "deserve" a dedicated switch, it gets kicked.

Unfortunately, this will kick out neat features like SSG gibbs or chainsaw/fist switching. On the other hand, most of these have either been merged into Doom Retro already or even taken from there. Altogether, I believe Doom Retro already fills the "Vanilla with Vanilla-incompatible enhancements" niche quite well.

Thoughts?

plumsinus commented 10 years ago

Well, my thoughts depend on what features will be removed ;)

But really, is your motivation to reduce features/differences from vanilla in general, or just to avoid features which are not vanilla-compatible? If the latter, is it worth trying to re-implement some features in a way that is vanilla-compatible? For instance I think corpse flipping could be accomplished in a vanilla-compatible way, possibly.

As for new features, I think it's a fine policy. There is very little I can think of that I would add to Crispy Doom at this point anyhow.

Relatedly, I've started hacking at the Crispy Doom source a bit. I haven't set myself up to start making branches on github, and I don't have anything worth sharing at this point anyhow. But, things like removing singleplayer conditionals and then testing demo sync are certainly things I can do.

On that note, are you aware of any situations in which demo sync would always be preserved, but netgame sync would not be? Testing features for demo compatibility is quite easy for me, netgame sync is another matter though.

fabiangreffrath commented 10 years ago

But really, is your motivation to reduce features/differences from vanilla in general, or just to avoid features which are not vanilla-compatible?

Hm, more the latter I guess.

Actually, I want to avoid developing two different feature sets - one for regular play and one for demos/netplay. I want to play the exact same game (with a few exceptions, of course) whether recording a demo or not. Features that contradict to this and are not drastic enough to deserve a dedicated switch in crispy-doom-setup should leave. Maybe I overexaggerated a bit when I wrote "most of these features".

For instance I think corpse flipping could be accomplished in a vanilla-compatible way, possibly.

I think it already is and this is one of the features I absolutely want to keep. the corpse flipping is decided upon the health of the already dead monster and I don't think this value is used anywhere else. So the "if (singleplayer)" added around it has to leave. Similar for the bumping ammo left by shot zombiemen. I think adding the singleplayer check there was over-cautious, but I don't yet have a test case to be absolutely sure.

There is very little I can think of that I would add to Crispy Doom at this point anyhow.

Think harder, HARDER! :)

But, things like removing singleplayer conditionals and then testing demo sync are certainly things I can do

Yes, please. The checks should be kept arond the code that determines level progression and par times for the BFG Edition IWADs and especially NERVE.WAD (since both PrBoom+ and Chocolate Doom do not special-case these). All the others should probably leave - either the check must leave or the feature it hides from demo recording.

I am going to go through an extensive code review anyway, so I expect to put any of these checks in question within the next release cycle. Then we should test demo compatibility extensively.

plumsinus commented 10 years ago

I grepped for singleplayer in the Crispy source, here's what came up and my take on it.

Enhancements with options already, can be left as is:

NERVE.WAD stuff and BFG Edition stuff, should be left as-is:

Cheat code related:

Visual enhancements - I will test all of these relatively soon.

Gameplay enhancements:

Bug fixes:

fabiangreffrath commented 10 years ago

I grepped for singleplayer in the Crispy source, here's what came up and my take on it.

Thank you very much for this! Incidently, I am doing mostly the same right now. ;)

Enhancements with options already, can be left as is:

agreed

NERVE.WAD stuff and BFG Edition stuff, should be left as-is:

agreed, level progression and map items should be left untouched when recording a demo

SSG in Doom 1 - hopefully this can stay, I feel like including the SSG resources is somewhat equivalent to having a dedicated switch.

stays! using this in a demo would be equally dumb as cheating

Backpack on ID(K)FA - should go, since you can use cheats in multiplayer. (I think this is the only condition where demo sync and multiplayer sync differ?)

hm, prboom+ has this undoncitionally, iirc. however, as you state, might affect network games with choco. is cheating really possible in choco? the wqhole code is embraced in "if (!netgame)".

Eat key press, i.e. don't change weapon upon level change - might affect netgame sync, I'm not sure.

again, might screw up network games with choco. but then, it's a cheat, so you're mostly on your own anyway.

New cheats, i.e. TNTEM

are not available in network anyway, using them in demos will screw them up just as any other cheat code

fabiangreffrath commented 10 years ago

Vertical ammo bob from dropped ammo - I think I can come up with an edge case where this would cause desync, unfortunately: killing a trooper in a pit higher than 16 units and running over the item as it bobs up.

I think collision detection is only done horizontally, i.e. if you run "over" an item you grab it, no? How about gibbing a tropper with the RL, the ammo gains both horizontal (from the gibbing) and vertical (from the bob) momentum and bobs on a nearby edge? Not sure, will have to see if this really affects demos.

SSG gibbing - unsure.

must most certeainly go

Lost souls bleed puffs - unsure, no great loss if it goes though.

I believe spawning Blood and Puffs does the same access to the RNG, so should be safe

Weapon sprite centring when shooting - probably OK?

The vertical centering causes desync, because it affects weapon lowering ans raising. Horizontal centering should be safe.

Corpse flipping/randomize corpse health on dying - probably OK?

i think so.

E4 par times - should be fine?

In general yes, but affects the output of the "statcheck" utility to compare demo sync. Should probably be made even stricter.

fabiangreffrath commented 10 years ago

Allow non-berserk fist with chainsaw - should be removed.

bye!

Never start map with fist if you have chainsaw - should be removed.

bye!

Auto-reload from save after dying - should be OK since you can't save/load in a demo anyhow?

no idea, but I like it!

Secret exit on MAP02->MAP33 - would cause a desync in a multi-level movie that takes a MAP02 secret exit, but maybe it's enough of an edge case & related to BFG edition to overlook? D2TWID does have a MAP02 secret exit.

yes, still not sure. i'd say, level progression should be left untouched in demos. c.f. prboom+ and choco

// [crispy] force weapon switch if weapon not owned - don't know what this is for, maybe just SSG in Doom 1?

this is related to removing the current weapon with TNTWEAP cheat. should be impossible to trigger else, should stay

key_menu_nextlevel/key_menu_reloadlevel - ?

shortcuts for IDCLEV, should be treated as such

Clear twosided flag on single-sided lines, i.e. for NOVA MAP30 - includes a comment from entryway about not clearing this flag for compatibility purposes.

important! unfortunately, the check in the release does not work, as the map is loaded before demorecording/demoplayback vriables are evaluated. will need a different check.

plumsinus commented 10 years ago

is cheating really possible in choco?

Apparently not, I must have remembered wrong. Cheats can stay as-is then.

I think collision detection is only done horizontally, i.e. if you run "over" an item you grab it, no?

Nope. You can test fairly easily on Doom 2 MAP29: run to the medikit to start the platform lowering and then move backwards quickly so you don't go down with it, wait for it to lower, and then fall on the chaingun; you won't pick it up until you actually land on it.

fabiangreffrath commented 10 years ago

Auto-reload from save after dying - should be OK since you can't save/load in a demo anyhow?

you can, but things are getting weird. however, this somehow affect "progression" and should thus be conditional. also, i might invert the check for the "run" key back to Vanilla behavious when it is not pressed

Nope. [...] you won't pick it up until you actually land on it.

damn, so this is a hot candidate for removal. but i'd like to keep it in until we've found a non-acadamic "might be" case

plumsinus commented 10 years ago

damn, so this is a hot candidate for removal. but i'd like to keep it in until we've found a non-acadamic "might be" case

Given that it has been in Crispy Doom for a while, and we both seem to like it, it might also be a candidate for exception from purging of non-drastic enhancements. Anyhow I'll test this one first, today or tomorrow.

fabiangreffrath commented 10 years ago

E4 par times - should be fine? In general yes, but affects the output of the "statcheck" utility to compare demo sync. Should probably be made even stricter.

just checked par times: (gamemap == 33) should not be possible in a regular demo (at least nothing to compare "statcheck" against), (gamemission == pack_nerve) is only possible with the BFG Edition IWADs which are known to cause desyncs anyway, the rest is already safe.

plumsinus commented 10 years ago

Regarding ammo bobbing: I have watched a few demos made with PrBoom+, using Crispy Doom with ammo bobbing always on (removing the conditional from line 777 of p_inter.c), and have not seen a desync yet. I feel like this use case is very unlikely to produce a desync. However the opposite case - making a recording with Crispy Doom that has ammo bobbing, and playing it back in a port that does not - seems to me to be more likely to desync. I can't really test this in a non-academic way though, because such recordings don't currently exist.

I did manage to easily make a map+demo that desyncs in ports without ammo bobbing, if you wanted to see it. It will only exit the level properly if ammo bobbing is enabled always. https://www.mediafire.com/?acd28ldiv2diddw

One idea I had, that you may or may not like, is to group several not-very-drastic changes such as ammo bobbing under a single switch.

plumsinus commented 10 years ago

Corpse flipping (remove singleplayer conditional from p_inter.c line 738 and r_things.c line 589): Seems to work fine with no desyncs, tested a few demos that have lots of monsters and many arch-viles that resurrect corpses.

plumsinus commented 10 years ago

Lost souls bleed puffs (p_mobj.c line 842): no desyncs after removing the conditional.

Incidentally, I had to record my own test cases for this, as (a) most demo players ignore lost souls, since they don't count as kills, and (b) lost souls spit by pain elementals still bleed blood! Will file a new bug about that one.

plumsinus commented 10 years ago

Weapon centring (p_pspr.c lines 286-290): no desyncs after removing the conditional and the y-axis change. I actually like the visual effect that results from this, where the weapon is always centred when firing but will appear at different heights.

fabiangreffrath commented 10 years ago

So, we have so far come to the same conclusions - which is great!

Thank you for the ammobob test case. Unfortunately, this will be the nail to the coffin for that feature. :/

fabiangreffrath commented 10 years ago

One idea I had, that you may or may not like, is to group several not-very-drastic changes such as ammo bobbing under a single switch.

Rather no, sorry, that's exactly the feature disparity that I want to avoid.

Plus, I believe we already have too many switches. For example, I'd like to get rid of the one for "free look" and unconditionally enable it (you can still just not use it, if you don't like it), but this also implicitely affects sky stretching -- which I'd like to keep intact if "free look" is not desired. Similarly, I'd like to get rid of the "enable jumping" switch and just unconditionally enable it (if you don't want to jump, well, just don't do it), but this also implicitely affects in-air movement control, which is highly demo-critical and must of course be disabled by default.

A new switch should be bound to a feature that really adds to the game's feel (such as the latest addition: recoil), not something you'll barely notice -- and especially not combine a bunch of features you'll barely notice under one label "enable features you'll barely notice but that will screw up your demos". ;)

plumsinus commented 10 years ago

that's exactly the feature disparity that I want to avoid.

No problem, I thought that might be the case, but thought I'd suggest it anyhow.

Plus, I believe we already have too many switches.

You could probably merge "Show level stats in automap" and "Show secret revealed message" into one option.

fabiangreffrath commented 10 years ago

You could probably merge "Show level stats in automap" and "Show secret revealed message" into one option.

Hm, are they so similar? How would that switch be called, "Show additional information in HUD" maybe?

plumsinus commented 10 years ago

I think they're similar enough in function that I don't see anyone getting too upset about them getting merged. (I hope :p)

"Show level stats and secret message" might work.

fabiangreffrath commented 10 years ago

But this will brake everyone's config. :/

Maybe I can drop the "highlight laserpointer" switch once I've implemented the "real" sprite-based laserpointer.

plumsinus commented 10 years ago

Well you could use crispy_automapstats for the config option still, and just drop crispy_secretmessage. But anyhow I don't care so much personally, it seemed to me like a safe thing to merge the two options but if you disagree then ok :)