Tsuey / L4D2-Community-Update

Help us shape the potential future of L4D2 vanilla.
70 stars 23 forks source link

Improve game mode selectivity of VScript God spots #507

Closed alexiscoutinho closed 5 months ago

alexiscoutinho commented 5 months ago

By submitting, I acknowledge the topic has been discussed (issues or elsewhere), that I know what is required of compiled assets (CONTRIBUTING.md -> Coordination), and if these are text or script files I'll submit un-modified live game files first.

What exactly is changed and why?

This PR aims to reduce the collateral damage of the newly introduced VScript God spots of 2f44245 in #479 following the spirit of #505. It simply optimizes the if else clauses and activates the God spots only in Campaign and Realism modes (the ones used by speedrunners), thus excluding almost all mutations, which never signed up for that new feature.

Is there anything specific that needs review?

No, though I haven't tested it ingame since the changes are so basic.

Does this address any open issues?

No.

Tsuey commented 5 months ago

Appreciate the thought, thanks! We'll be closing this one in favor of getting #505 through some final stages of review which, even with some limitation, is our response to a complex discussion about re-introducing some godspots that TLS fixed. There's also another pending PR (not yet submitted but will be merged) that touches most of the map scripts, so we're also acting to mitigate merge conflicts.

Overall gist is that we want to reduce the ability for a Survivor to "grief" their teammates by standing on godspots, and support speedrunners since TLS added a new campaign and they need the current version to speedrun the full game. Lots of discussion feeds into the other PR, and is the best compromise so far.

alexiscoutinho commented 5 months ago

I don't know if you fully understood the gist of this PR. It's not meant to be an alternative to #505, it's meant to be an enhancement/extra. If we assume that the reintroduction of finale Godspots was only to aid the speedrunning community, then there's no objective reason to reject this PR idea. Let me be clear, this PR basically only impacts mutations. Afaik, nobody seriously speedruns mutations, with leaderboards and such, as that's kinda against the point. If so, there would be no reason to reintroduce Godspots in those cases. Correct me if my assumptions are wrong.

In fact, the only objective argument against this specific PR would be the merge conflict one. Albeit it's such a minor thing under my responsibility (i.e. queue this PR until the indev one is merged). Something I could easily do if necessary, even rebase if the conflict gets nasty.

Therefore, I'm a bit confused with your reply overall. Regards.

Tsuey commented 5 months ago

The merge conflicts is the big one as Failzz' branch contributes a significant amount and there is no ETA on when that would be merged and is high priority, which would have resulted in this PR sitting in limbo for potentially months, and it's in our contributing guidelines that PR's be discussed before they're submitted. We also expect PR's to be tested by the author, as "basic" can be subjective.

The file c12m5_cornfield.nut is only modified here for whitespace, and overall the else/if don't cover every single case for the Mutations changed, and some if statements are removed entirely in favor of else adding inconsistencies (i.e. why are all map scripts not modified in the same way). After the Tank Run outlash with the Refresh patch, "nobody speedruns Mutations" is not a strong bet anymore since we've been surprised once before, so that area would need extra scrutiny as well.

ImAciidz commented 5 months ago

Correct me if my assumptions are wrong.

People speedrun rocketdude and other third party mutations (within official leaderboards).

alexiscoutinho commented 5 months ago

which would have resulted in this PR sitting in limbo for potentially months,

I see... Well, the first commit changes ~70 lines, so I can see why others may prefer to not bother with the conflicts/changes in their branch. So the only option would be for me to wait for the other big changes to be pushed, which I don't mind.

it's in our contributing guidelines that PR's be discussed before they're submitted.

Well, we might have a small issue here. I don't know if the current development environment is the best. It seems a bit too centralized, i.e. one would need to ask for permission for each and every independent change, including minor ones like this PR. Maybe somehow encouraging more semi-public discussions would help let others be in the same page and not rely so much on intermediaries (those 6). This is just something to think about, I'm not really demanding anything.

We also expect PR's to be tested by the author, as "basic" can be subjective.

Of course, but I still maintain my position on this one. The commits, especially the first one, are indeed basic and boring changes that rely purely on programming logic (and basic knowledge of what g_BaseMode and g_MutaMode mean). One could have 0 knowledge about Left 4 Dead 2 or even Squirrel and still be able to attest that the versions are equivalent (first commit), if they feel confident about their logic knowledge.

The file c12m5_cornfield.nut is only modified here for whitespace,

Yes, I went through all the map fixes files and also ensured whitespace consistency in those specific lines. These files are all new and would need to be fully diffed by Kerry anyways.

the else/if don't cover every single case for the Mutations changed, and some if statements are removed entirely in favor of else adding inconsistencies (i.e. why are all map scripts not modified in the same way).

They (if/else changes) cover all the applicable cases. Depending on the surrounding if clauses, one can substitute with else if, else or even be unable to make any change at all. Therefore the first commit is no more inconsistent than the if clauses' structures already were. It's clear though that those changes are very context dependent, i.e. if one adds another mode check, then the else clause might not be appropriate anymore. As such, the big pending Failzz PR may significantly change the optimal if/else structure. Therefore the idea of the first commit is more adequate when the map fixes files reach a more finished state.

After the Tank Run outlash with the Refresh patch, "nobody speedruns Mutations" is not a strong bet anymore since we've been surprised once before, so that area would need extra scrutiny as well.

👍. Regarding this matter (affecting second commit), I can only lament at this stage. I still generally frown upon Godspot usage, but I'll leave this at that.