coelckers / prboom-plus

This is a cleaned up copy of the PrBoom+ SVN repository as a courtesy for those interested in forking that port
292 stars 117 forks source link

Number 3 on STARMS doesn't highlight when the player only has the super shotgun #184

Closed maxmanium closed 3 years ago

maxmanium commented 3 years ago

Old vanilla bug.

fabiangreffrath commented 3 years ago

This is one of the cases where I am not sure if we should keep the bug for consistency or fix it for convenience.

fabiangreffrath commented 3 years ago

That would be the patch:

--- a/prboom2/src/st_stuff.c
+++ b/prboom2/src/st_stuff.c
@@ -354,6 +354,9 @@ static st_binicon_t  w_armsbg;
 // weapon ownership widgets
 static st_multicon_t w_arms[6];

+// [crispy] show SSG availability in the Shotgun slot of the arms widget
+static int st_shotguns;
+
 // face status widget
 static st_multicon_t w_faces;

@@ -815,6 +818,9 @@ static void ST_drawWidgets(dboolean refresh)
   // used by w_frags widget
   st_fragson = deathmatch && st_statusbaron;

+  // [crispy] show SSG availability in the Shotgun slot of the arms widget
+  st_shotguns = plyr->weaponowned[wp_shotgun] | plyr->weaponowned[wp_supershotgun];
+
   //jff 2/16/98 make color of ammo depend on amount
   if ((*w_ready.num == plyr->maxammo[weaponinfo[w_ready.data].ammo]) ||
     (ammo_colour_behaviour == ammo_colour_behaviour_no && plyr->backpack &&
@@ -1093,6 +1099,8 @@ static void ST_createWidgets(void)
                          arms[i], (int *) &plyr->weaponowned[i+1],
                          &st_armson);
     }
+  // [crispy] show SSG availability in the Shotgun slot of the arms widget
+  w_arms[1].inum = &st_shotguns;

   // frags sum
   STlib_initNum(&w_frags,
fabiangreffrath commented 3 years ago

Before I file a PR: What's our general stance towards bugs or features like this, i.e. the ones that are trivial to fix but probably remain unfixed just because "they have always been like that". I mean, fixing an issue like this has zero impact on game logic and compatibility but probably increase QoL for the player, while on the other hand introduces another break with the strict faithfulness of the port.

I see three choices:

  1. Fix all the things!
  2. Leave eveything as is.
  3. Introduce a menu switch for every fix.

What do you guys say?

JadingTsunami commented 3 years ago

3. Introduce a menu switch for every fix.

My feeling is this option is most in-line with the original Boom philosophy. This creates an ocean of menu options most of which (if we're being honest) are rarely used, but it gives the player full control over how "authentic" they want their experience to be.

fabiangreffrath commented 3 years ago

Alright. Does this rather belong into the "Weapons" or the "Status Bar / HUD" section?

kraflab commented 3 years ago

I don't feel like this is a good use of menu real estate personally 🤠

kraflab commented 3 years ago

To play devil's advocate:

Before "fix": highlight indicates if player has regular shotgun

After "fix": highlight indicates if player has either shotgun

Is that correct? I'd say there is a change of meaning there, not a bug fix. The amount of information available to the player is conserved, but they have different information.

fabiangreffrath commented 3 years ago

So, what does this mean now?

kraflab commented 3 years ago

To be honest @fabiangreffrath I think this is kind of a decision for you to make - you're the de facto maintainer of this fork and can decide what the rules are going to be based on where you want to take it. The essential goal of this fork itself was just to support umapinfo as far I know, so beyond that is uncharted territory. You could just as easily freeze the code to these kinds of changes or embrace them. On this specific topic I would personally vote "don't fix" but who am I? 🤷

maxmanium commented 3 years ago

From my frame of reference it's a bug. It's speculation, but honestly I think the guys at id just forgot to add the check for the SSG when they added it with Doom II, since they weren't really changing much with the status bar code, presumably.

JadingTsunami commented 3 years ago

Yes, I think @kraflab is right. In the end the port maintainer can decide the vision.

I would recommend fixing bugs (and adding compat flags where appropriate) but leaving the core game logic frozen for enhancements beyond UMAPINFO.

But this is only my suggestion. Where the line between "enhancement" and "bug fix" is drawn is a matter of opinion more than fact.

fabiangreffrath commented 3 years ago

Guys, I know that I am considered to be the new maintainer of this fork and it's true that I am currently the most active contributor. However, I don't want to drive this project into "my direction" - in fact, I don't have a direction for this. I just want to increase the port's QOL value and add changes requested by users. I don't want to decide about what's wrong or right, I want this to be based on community consent. That's why I am asking for approval for any bigger change. (Sorry for sounding pathetic 😉 )

kraflab commented 3 years ago

I completely understand that - the github audience is probably not representative of the community though. Distributing the influence over the port to people on here is it's own choice and inevitably pushes things in a certain direction (which may or may not be the direction the wider "community" would choose). Keep in mind a bunch of people on the forums also want this fork to start incorporating zdoom features or drop complevel support, which I think we can agree is not the right choice.

As an example, blood colors is, to me, completely out of scope for prboom+ and I wouldn't have considered it. But there are other people who do want that change. I wouldn't outright resist a change beyond voicing my opinion unless I think it's legitimately destructive (e.g., removing a complevel). Each person has a different idea for what the direction or goal is. Ultimately the one who clicks merge is unavoidably making the decision. If you don't want to adopt that responsibility, then I would err on the side of being extra conservative. I think people are generally happy with what you've been achieving though, so the current strategy is working 😸

fabiangreffrath commented 3 years ago

As an example, blood colors is, to me, completely out of scope for prboom+ and I wouldn't have considered it. But there are other people who do want that change.

Yes, I know. To me this change is also borderline, but there is something special about that colored blood feature that makes people just love it. Ever since I posted this preliminary patch here [1] some 7-8 years ago, people keep asking me or posting on the forum how to enable that feature for PrBoom+ (or how to apply the patch to the executable) or if I could provide a build with this one patch applied. Granted, in principle, I wouldn't consider this feature suitable for PrBoom+ either, but I know that some people will love it and would continue asking for it otherwise.

[1] https://sourceforge.net/p/prboom-plus/feature-requests/91/

I wouldn't outright resist a change beyond voicing my opinion unless I think it's legitimately destructive (e.g., removing a complevel).

Thank you and please keep this up! I depend on a second opinion about some changes and I am glad each time you express yours - even if it's just clicking the "approve" button of a pull request. You know, this port is not my baby, I just came here as a random contributor and then somehow become more.

For Crispy, for example, I feel I can do whatever I want and I have a clear vision of what's possible and what not. I am a bit more conservative with Woof!, though, because I started this project to continue someone else's vision of a Doom port. But still, I feel I have to be very careful with everything I do here, not alone because of the role this port plays in the general community but especially among speed runners.

Each person has a different idea for what the direction or goal is. Ultimately the one who clicks merge is unavoidably making the decision. If you don't want to adopt that responsibility, then I would err on the side of being extra conservative. I think people are generally happy with what you've been achieving though, so the current strategy is working smile_cat

I am fine with taking the responsibility to apply a set of patches. But it feels a lot better if you know that someone else has approved these changes as well, or would have done the same. However, I also have the feeling that people are in general happy with what we have achieved with this port so far - so the current way we handle things here can't be that wrong. :grin:

maxmanium commented 3 years ago

Is there a consensus on this?

fabiangreffrath commented 3 years ago

Is there a consensus on this?

Well, no.

runlow commented 3 years ago

What's requested in this issue already applies to the althud (press + twice, - once to toggle back and forth). When you pick up the SSG - a "9" appears after "WEA". This can be easily tested on map05 in DOOM2.WAD as there's an SSG right in front of the player at the start of the map. The althud has many other useful things you can't see on the classic HUD. If needing this info - one could toggle the althud to check - or have althud on always.

kraflab makes a good point about the proposed enhancement/fix changing the meaning of the "3". No other shown number corresponds to "multiple weapons in one slot" so there's no way to tell how it was intended instead, and if this behaviour wasn't intentional. Given that there's already a way to check if one has the SSG (althud) - I think the current behaviour shouldn't be changed.

I'm happy with the overall direction of the project personally.

fabiangreffrath commented 3 years ago

Agreed. We already have other means to provide more detailed weapon availability information. No need to become non-canonical here.