0001277: LMS weapon rework

(0005982)
StrikerMan780 (reporter)
2013-02-10 17:10

I strongly support this, as this was done as a result of some testing done between Dusk and I.

(0005985)
Torr Samaho (administrator)
2013-02-10 19:56

Anything that changes the existing LMS mechanics should be run through a community vote. Adding support for more IWADs is no problem of course, but any changes that alter the LMS gameplay for IWADs that are already supported should only be made if the majority of players want this to be changed.

(0005988)
Dusk (developer)
2013-02-10 21:35
edited on: 2013-02-10 23:44

The behavior should be mostly identical in Doom and Heretic... the only change that affects Heretic is that berserk is given in it (which Heretic doesn't even use, I added that behavior for consistency and that alone) and that the beak isn't given in LMS (and if this really is such a big deal I guess it can just be allowed again). What exactly are the problematic changes here?

(0006005)
Torr Samaho (administrator)
2013-02-11 17:48

I was making a general statement. From the description I wasn't completely sure which aspects may be changed. So in Doom everything is supposed to behave identically?

Since PowerStrength is not used in Heretic, I think it's kinda strange to give it there.

(0006006)
Dusk (developer)
2013-02-11 20:26

Doom and Heretic LMS should be behaving identically, yes. PowerStrength is given for the sake of consistency but I guess it can be gametype'd out if needed. My only concern is that that modders might've used features in ways which would cause behavioral changes in some form or another but I haven't run into such cases.. at least yet.

(0006012)
StrikerMan780 (reporter)
2013-02-12 06:01

I make use of berserk in SMMP's heretic support, so keeping that in would be a good idea.

(0006019)
Torr Samaho (administrator)
2013-02-13 06:30
edited on: 2013-02-13 06:30

Neither Skulltag nor Zandronum 1.0 give you berserk in Heretic LMS, right?

(0006020)
Dusk (developer)
2013-02-13 11:32

No, it was something explicitly given for the DoomPlayer. Hence:

Quote
My only concern is that that modders might've used features in ways which would cause behavioral changes in some form or another but I haven't run into such cases.. at least yet.

I don't even know of any LMS mods for Heretic to begin with.

(0006021)
Torr Samaho (administrator)
2013-02-13 20:35

Yeah, that's how I understood your description. Just

Quote from StrikerMan780
I make use of berserk in SMMP's heretic support, so keeping that in would be a good idea.

sounded as if StrikerMan780 already relies on berserk in Heretic. But that would require berserk to be given in Heretic which it is not. StrikerMan780, can you clarify your statement?

(0006022)
StrikerMan780 (reporter)
2013-02-13 20:39

Well, In standard gameplay I make use of berserk, as it is randomly spawned in the place of the Gauntlets. It would be helpful for it to be given to you from the start in LMS if played under Heretic.

(0006063)
Torr Samaho (administrator)
2013-02-23 09:27

Since Berserk is not a Heretic item and it is not given in Zandronum 1.0 or any Skulltag version in Heretic LMS, I'd say we should not change this behavior.

General remarks regarding the patch:
- Please separate the buckshot and instagib merging from the LMS changes. These things are completely independent from the LMS overhaul and should go to a separate commit. Also after a quick look I think your merging changed the behavior. Previously instagib and buckshot were mutually exclusive, now it seems you can activate both at once. We may want to allow this, but if so, we should make this intentionally.
- IMHO using labels should be considered a last ditch effort in C++. Most of the time using a label is an indication that the code should be redesigned. Looking at donewitharmor for example, it would be much cleaner if you move the armor giving code into a function and replace all "goto donewitharmorr" with return.
- DF2_COOP_SHOTGUNSTART only affected DoomPlayer before, now it is applied to all player classes, independently of the used IWAD
- The patch still looks unfinished, there are numerous debug outputs, so I didn't dissect everything in detail so far.

(0006103)
Dusk (developer)
2013-03-10 22:56

'https://bitbucket.org/CrimsonDusk/neurosphere/commits/493dca7001e939f2ab549d667a0ea99c78f4e2fe [^]'
Better now?

(0006138)
Torr Samaho (administrator)
2013-03-17 19:30

Looks better. Some more questions:

- Did you intentionally remove the pType->ActorInfo check from "if ( ( pType->ActorInfo->GameFilter != GAME_Any ) && ( ( pType->ActorInfo->GameFilter & gameinfo.gametype ) == 0 ) )"?

- Are

if ( pWeapon->Ammo1 )
                        {
                            pInventory = player->mo->FindInventory( pWeapon->Ammo1->GetClass( ));

                            // Give the player the maximum amount of this type of ammunition.
                            if ( pInventory != NULL )
                                pInventory->Amount = pInventory->MaxAmount;
                        }

and

if ( pWeapon->Ammo1 )
pWeapon->Ammo1->Amount = pWeapon->Ammo1->MaxAmount;

equivalent?

(0006140)
Dusk (developer)
2013-03-17 21:34

Quote
- Did you intentionally remove the pType->ActorInfo check from "if ( ( pType->ActorInfo->GameFilter != GAME_Any ) && ( ( pType->ActorInfo->GameFilter & gameinfo.gametype ) == 0 ) )"?

The code filters out all non-AInventory types as the first check it performs. Anything that is an AInventory should also be an AActor and thus have ActorInfo, right?

Quote
- Are [...] and [...] equivalent?

I believe they are. I don't understand why is it getting the ammo's type and then finding it and setting it when it could just set the ammo directly. I've tested this code a lot and I didn't have a case of not getting full ammo.

Only registered users can voice their support. Click here to register, or here to log in.
Supporters:	ZzZombo StrikerMan780 Balrog Ivan
Opponents:	No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2013-02-09 22:43	Dusk	New Issue
2013-02-09 22:43	Dusk	Status	new => assigned
2013-02-09 22:43	Dusk	Assigned To	=> Dusk
2013-02-09 22:43	Dusk	Status	assigned => feedback
2013-02-09 22:46	Dusk	Description Updated	View Revisions
2013-02-10 17:10	StrikerMan780	Note Added: 0005982
2013-02-10 19:56	Torr Samaho	Note Added: 0005985
2013-02-10 21:35	Dusk	Note Added: 0005988
2013-02-10 21:35	Dusk	Status	feedback => assigned
2013-02-10 21:36	Dusk	Status	assigned => feedback
2013-02-10 21:40	Dusk	Note Edited: 0005988	View Revisions
2013-02-10 23:44	Dusk	Note Edited: 0005988	View Revisions
2013-02-11 17:48	Torr Samaho	Note Added: 0006005
2013-02-11 20:26	Dusk	Note Added: 0006006
2013-02-11 20:26	Dusk	Status	feedback => assigned
2013-02-12 05:46	Dusk	Assigned To	Dusk =>
2013-02-12 05:46	Dusk	Status	assigned => feedback
2013-02-12 06:01	StrikerMan780	Note Added: 0006012
2013-02-12 08:20	Dusk	Assigned To	=> Dusk
2013-02-13 06:30	Torr Samaho	Note Added: 0006019
2013-02-13 06:30	Torr Samaho	Note Edited: 0006019	View Revisions
2013-02-13 06:31	Torr Samaho	Note Revision Dropped: 6019: 0003318
2013-02-13 11:32	Dusk	Note Added: 0006020
2013-02-13 11:32	Dusk	Status	feedback => assigned
2013-02-13 11:33	Dusk	Status	assigned => feedback
2013-02-13 20:35	Torr Samaho	Note Added: 0006021
2013-02-13 20:39	StrikerMan780	Note Added: 0006022
2013-02-23 09:27	Torr Samaho	Note Added: 0006063
2013-02-24 12:51	Dusk	Status	feedback => assigned
2013-03-10 22:56	Dusk	Note Added: 0006103
2013-03-10 22:56	Dusk	Status	assigned => needs review
2013-03-17 19:30	Torr Samaho	Note Added: 0006138
2013-03-17 19:31	Torr Samaho	Status	needs review => feedback
2013-03-17 21:34	Dusk	Note Added: 0006140
2013-03-17 21:34	Dusk	Status	feedback => assigned
2019-06-04 21:54	Dusk	Status	assigned => new

Notes
(0005982) StrikerMan780 (reporter) 2013-02-10 17:10	I strongly support this, as this was done as a result of some testing done between Dusk and I.

(0005985) Torr Samaho (administrator) 2013-02-10 19:56	Anything that changes the existing LMS mechanics should be run through a community vote. Adding support for more IWADs is no problem of course, but any changes that alter the LMS gameplay for IWADs that are already supported should only be made if the majority of players want this to be changed.

(0005988) Dusk (developer) 2013-02-10 21:35 edited on: 2013-02-10 23:44	The behavior should be mostly identical in Doom and Heretic... the only change that affects Heretic is that berserk is given in it (which Heretic doesn't even use, I added that behavior for consistency and that alone) and that the beak isn't given in LMS (and if this really is such a big deal I guess it can just be allowed again). What exactly are the problematic changes here?

(0006005) Torr Samaho (administrator) 2013-02-11 17:48	I was making a general statement. From the description I wasn't completely sure which aspects may be changed. So in Doom everything is supposed to behave identically? Since PowerStrength is not used in Heretic, I think it's kinda strange to give it there.

(0006006) Dusk (developer) 2013-02-11 20:26	Doom and Heretic LMS should be behaving identically, yes. PowerStrength is given for the sake of consistency but I guess it can be gametype'd out if needed. My only concern is that that modders might've used features in ways which would cause behavioral changes in some form or another but I haven't run into such cases.. at least yet.

(0006012) StrikerMan780 (reporter) 2013-02-12 06:01	I make use of berserk in SMMP's heretic support, so keeping that in would be a good idea.

(0006019) Torr Samaho (administrator) 2013-02-13 06:30 edited on: 2013-02-13 06:30	Neither Skulltag nor Zandronum 1.0 give you berserk in Heretic LMS, right?

(0006020) Dusk (developer) 2013-02-13 11:32	No, it was something explicitly given for the DoomPlayer. Hence: Quote My only concern is that that modders might've used features in ways which would cause behavioral changes in some form or another but I haven't run into such cases.. at least yet. I don't even know of any LMS mods for Heretic to begin with.

(0006021) Torr Samaho (administrator) 2013-02-13 20:35	Yeah, that's how I understood your description. Just Quote from StrikerMan780 I make use of berserk in SMMP's heretic support, so keeping that in would be a good idea. sounded as if StrikerMan780 already relies on berserk in Heretic. But that would require berserk to be given in Heretic which it is not. StrikerMan780, can you clarify your statement?

(0006022) StrikerMan780 (reporter) 2013-02-13 20:39	Well, In standard gameplay I make use of berserk, as it is randomly spawned in the place of the Gauntlets. It would be helpful for it to be given to you from the start in LMS if played under Heretic.

(0006063) Torr Samaho (administrator) 2013-02-23 09:27	Since Berserk is not a Heretic item and it is not given in Zandronum 1.0 or any Skulltag version in Heretic LMS, I'd say we should not change this behavior. General remarks regarding the patch: - Please separate the buckshot and instagib merging from the LMS changes. These things are completely independent from the LMS overhaul and should go to a separate commit. Also after a quick look I think your merging changed the behavior. Previously instagib and buckshot were mutually exclusive, now it seems you can activate both at once. We may want to allow this, but if so, we should make this intentionally. - IMHO using labels should be considered a last ditch effort in C++. Most of the time using a label is an indication that the code should be redesigned. Looking at donewitharmor for example, it would be much cleaner if you move the armor giving code into a function and replace all "goto donewitharmorr" with return. - DF2_COOP_SHOTGUNSTART only affected DoomPlayer before, now it is applied to all player classes, independently of the used IWAD - The patch still looks unfinished, there are numerous debug outputs, so I didn't dissect everything in detail so far.

(0006103) Dusk (developer) 2013-03-10 22:56	'https://bitbucket.org/CrimsonDusk/neurosphere/commits/493dca7001e939f2ab549d667a0ea99c78f4e2fe [^]' Better now?

(0006138) Torr Samaho (administrator) 2013-03-17 19:30	Looks better. Some more questions: - Did you intentionally remove the pType->ActorInfo check from "if ( ( pType->ActorInfo->GameFilter != GAME_Any ) && ( ( pType->ActorInfo->GameFilter & gameinfo.gametype ) == 0 ) )"? - Are if ( pWeapon->Ammo1 ) { pInventory = player->mo->FindInventory( pWeapon->Ammo1->GetClass( )); // Give the player the maximum amount of this type of ammunition. if ( pInventory != NULL ) pInventory->Amount = pInventory->MaxAmount; } and if ( pWeapon->Ammo1 ) pWeapon->Ammo1->Amount = pWeapon->Ammo1->MaxAmount; equivalent?

(0006140) Dusk (developer) 2013-03-17 21:34	Quote - Did you intentionally remove the pType->ActorInfo check from "if ( ( pType->ActorInfo->GameFilter != GAME_Any ) && ( ( pType->ActorInfo->GameFilter & gameinfo.gametype ) == 0 ) )"? The code filters out all non-AInventory types as the first check it performs. Anything that is an AInventory should also be an AActor and thus have ActorInfo, right? Quote - Are [...] and [...] equivalent? I believe they are. I don't understand why is it getting the ammo's type and then finding it and setting it when it could just set the ammo directly. I've tested this code a lot and I didn't have a case of not getting full ammo.

Relationships