0000833: Certain effects in SetPlayerProperty are not working as intended online

(0003599)
TIHan (reporter)
2012-05-08 05:40

Fixed.
'https://bitbucket.org/TIHan/tst/changeset/8c0fe17ed9b7 [^]'
This should also take care of bug 832.

(0003634)
Torr Samaho (administrator)
2012-05-14 20:11

p_acs.cpp: I think the spectator checks in DLevelScript::SetActorProperty should be moved into DoSetActorProperty (then you only need the check once).

I need to look into the other changes in more detail. At first glance, the abBlendColor change looks very hackish.

(0003635)
TIHan (reporter)
2012-05-14 20:43
edited on: 2012-05-14 20:46

> p_acs.cpp: I think the spectator checks in DLevelScript::SetActorProperty should be moved into DoSetActorProperty (then you only need the check once).

Will do.

> At first glance, the abBlendColor change looks very hackish.

I couldn't decide what to do here. Other options for this are:

1. New Command: SERVERCOMMANDS_GivePowerupWithBlendColor - this will set the exact blend color ( the full 4 bytes ) that the powerup is using to clients.
2. Introduce an enum type with all the possible blend color types, and use this to send to clients in SERVERCOMMANDS_GivePowerup, or introduce a new command for this.
3. Use current solution but create SERVERCOMMANDS_GivePowerupWithBlendColor to handle it.

Solutions 1 and 2, to me, seem unecessary - as needing to know when the blend color changed on the server only occurs once in the entire codebase AFAIK.

(0003660)
Torr Samaho (administrator)
2012-05-27 10:47

I took care of the p_acs.cpp change and added the spectator part of your patch (with an added it->player check). We still need to think about abBlendColor.

(0003665)
TIHan (reporter)
2012-06-03 19:53

The question will be is how we will handle BlendColor itself on powerups. So far, the assignments to BlendColor are straight forward and happen in sync with the client and server (no need to tell clients about it). However, this one instance breaks it, so we need a way to tell the client that the BlendColor has been changed.

(0003670)
Torr Samaho (administrator)
2012-06-04 19:37

Why don't we do what we always do in such a case then and create a new SVC2 command that just sets the blend color? BTW: Since we can't introduce new commands for every single combination of possible server commands, I think we shouldn't introduce something like SERVERCOMMANDS_GivePowerUpWithBlendColor.

(0003671)
TIHan (reporter)
2012-06-04 19:47
edited on: 2012-06-04 19:51

> Why don't we do what we always do in such a case then and create a new SVC2 command that just sets the blend color?

IIRC, I don't think there is way to reference the PowerUp from the client if we were to use specifically a new command that sets the blend color of a PowerUp. PowerUps don't have NetIds? Edit: Perhaps we could do it by its class type?

(0003672)
Torr Samaho (administrator)
2012-06-04 20:49
edited on: 2012-06-04 20:50

Well, I didn't look at this in detail, but you destroy a PowerUp based on its netId in your patch:

if (power != 4)
                {
                    AInventory *item = it->FindInventory (powers[power]);
                    if (item != NULL)
                    {
                        // [WS] Destroy the powerup.
                        if ( NETWORK_GetState( ) == NETSTATE_SERVER )
                            SERVERCOMMANDS_DestroyThing( item );
                        item->Destroy ();
                    }
                }

So this part of your patch either doesn't work or it is how can reference the PowerUp on the client.

(0003673)
TIHan (reporter)
2012-06-04 20:57
edited on: 2012-06-04 20:58

> Well, I didn't look at this in detail, but you destroy a PowerUp based on its netId in your patch.

Then my patch wouldn't work and the powerup wouldn't actually be destroyed on the client. Looks like we are going to have to come up with IDs for inventory.
Edit: I can't look at anything right now. I will later today.

(0003674)
Torr Samaho (administrator)
2012-06-04 21:01

Removing inventory items can be done with SERVERCOMMANDS_TakeInventory.

(0003675)
TIHan (reporter)
2012-06-04 21:14

Then I guess it does it by inventory type in there. The amount is 0 and destroys it?

I question if there would be an edge case when setting the BlendColor by getting the powerup by inventory type.

(0003677)
Torr Samaho (administrator)
2012-06-05 21:15

What really worries me right now is the following: If SERVERCOMMANDS_DestroyThing has no effect where you put it in your patch, how did this change pass your testing?

BTW: I'm pretty sure that the inventory has a netID on the server. The server just doesn't tell it to the client when giving it.

(0003678)
TIHan (reporter)
2012-06-05 21:22
edited on: 2012-06-05 21:26

I think that SERVERCOMMANDS_DestroyThing or Destroy() never got fired in my testing. It would only get fired when you attempt to take away the powerup using a script argument(arg1) of 0. So this wouldn't be related to the original bug, but it was something I noticed and thought should be handled. Sorry for this confusion, as I could have mentioned my reasoning there.

So, a question to you, if inventory does have a netID, should we send it over to the client?

(0003684)
Torr Samaho (administrator)
2012-06-06 18:49

Oh, then I guess I didn't make clear that I assume all code submissions to be at least very briefly tested. This assumption is so fundamental to me, that I thought this goes without saying. So to the very least I think each code change should be tested at least once to confirm that they have the desired effect at least on one occasion (of course thorough testing would be much better). Putting completely untested code in a stable branch IMHO is a bad idea. This example here clearly shows how easily completely wrong things could get in otherwise. If for some reason you can't test a patch or parts of it but still really want to submit it, please make clear in the submission which parts are untested.

> So, a question to you, if inventory does have a netID, should we send it over to the client?

Not unless we absolutely have to. Of course sending it needs additional traffic and so far we managed to do everything without it. I guess identifying by name should be fine, or can you have the same item multiple times (instead of just once but with an amount higher than 1)?

(0003685)
TIHan (reporter)
2012-06-06 19:12
edited on: 2012-06-06 23:11

Then the whole issue was me not testing parts of my code changes, but only testing the bug itself. Fair enough. I will need to make changes to ACS in this case.

Edited again: Actually, it is fair to point out that I didn't test those pieces. I get it. However, I don't need a paragraph lecture about it. I'll correct whatever mistakes I make, and we move on. No sense in lecturing me this way.

I will attempt to try it by inventory type and see what occurs.

(0003713)
TIHan (reporter)
2012-06-09 18:40
edited on: 2012-06-09 18:40

'https://bitbucket.org/TIHan/zandronum/changesets/tip/branch(%22bug_833%22) [^]'

This solves the bug like you suggested.
I also uploaded a modified wad that tests specifically if the inventory item gets destroyed and it works properly.

(0003742)
Torr Samaho (administrator)
2012-06-12 19:27

The implementation of SERVERCOMMANDS_SetPowerupBlendColor is strange. If ( BlendColor != INVERSECOLOR ) it does absolutely nothing except for eating bandwidth. If you want to add a command that can nothing but set the blend color of a powerup to INVERSECOLOR (which should be sufficient here) it should be named as such and refuse to do anything in case ( BlendColor != INVERSECOLOR ). Unless you plan to implement a list of commonly used blend colors it's also not necessary to send abBlendColor. If you want a command that can more than just setting INVERSECOLOR, instead of a list, sending the actual PalEntry probably would be better though.

BTW: It would be very nice if you would commit separate things separately. For instance, the introduction of SERVERCOMMANDS_SetPowerupBlendColor has nothing to do with the NetCommand rewrite of SERVERCOMMANDS_GivePowerup, so having them in independent commits would be beneficial. And why did you remove the "Can we have multiple amounts of a powerup?" comment? Did you find out that one can have multiple amounts of a powerup?

(0003748)
TIHan (reporter)
2012-06-12 23:49
edited on: 2012-06-12 23:57

'https://bitbucket.org/TIHan/zandronum/changeset/7a42f5b395ad [^]'

This time I'm just sending the full PalEntry over. The check whether to send it or not will need to be outside the server command itself as you see "if ( power == 0 )" in p_lnspec.cpp. Before I had a check inside the server command to see if the blend color was 0, then don't send; however, I thought either at some point we may want to set the blend color to 0 and inform the clients in the future. For now, we only have one use case to use SetPowerupBlendColor, and that is exactly what it does regardless of what it is. Conserving bandwidth will be up to how you call it, rather than the command itself.

As far as having multiple powerups of the same type, it looks like you can't. From my understanding, when you receive a powerup of the same type that you already have, it will just reset the effecttics.

(0003762)
ZzZombo (reporter)
2012-06-14 07:35

Guys, are you aware of "Using this [SetPlayerProperty] special to grant powerup effects to players has been deprecated. Consider using the GiveInventory function for this purpose instead."? Maybe you just will deprecate it as well?

(0003768)
TIHan (reporter)
2012-06-14 14:27

> Maybe you just will deprecate it as well?
Even though it is deprecated, it is still used in some wads and we have to handle it.

(0003769)
ZzZombo (reporter)
2012-06-14 14:37
edited on: 2012-06-14 14:38

I think that's just pointless. If something is considered obsolete then that means nobody should ever use it. If one continues then he just made his choice and should deal with all side effects, especially if an proper alternative way is given.

(0003773)
Torr Samaho (administrator)
2012-06-14 18:54

Thanks! I added the patch with minor modifications ("break" instead of "return" and "PLAYER_IsValidPlayerWithMo" instead of "PLAYER_IsValidPlayer plus mo check").

(0004185)
Arcodios (reporter)
2012-08-02 01:51

Fixed.

This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why.
Supporters:	No one explicitly supports this issue yet.
Opponents:	No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2012-05-05 17:30	unknownna	New Issue
2012-05-05 17:30	unknownna	File Added: Screenshot_Doom_20120505_192610.png
2012-05-05 17:31	unknownna	Status	new => confirmed
2012-05-06 15:48	TIHan	Assigned To	=> TIHan
2012-05-06 15:48	TIHan	Status	confirmed => assigned
2012-05-08 04:29	TIHan	Relationship added	related to 0000832
2012-05-08 05:40	TIHan	Note Added: 0003599
2012-05-08 05:40	TIHan	Status	assigned => needs review
2012-05-14 20:11	Torr Samaho	Note Added: 0003634
2012-05-14 20:43	TIHan	Note Added: 0003635
2012-05-14 20:44	TIHan	Note Edited: 0003635	View Revisions
2012-05-14 20:45	TIHan	Note Edited: 0003635	View Revisions
2012-05-14 20:46	TIHan	Note Edited: 0003635	View Revisions
2012-05-27 10:47	Torr Samaho	Note Added: 0003660
2012-06-03 19:53	TIHan	Note Added: 0003665
2012-06-03 19:53	TIHan	Status	needs review => feedback
2012-06-04 19:37	Torr Samaho	Note Added: 0003670
2012-06-04 19:47	TIHan	Note Added: 0003671
2012-06-04 19:51	TIHan	Note Edited: 0003671	View Revisions
2012-06-04 20:49	Torr Samaho	Note Added: 0003672
2012-06-04 20:50	Torr Samaho	Note Edited: 0003672	View Revisions
2012-06-04 20:57	TIHan	Note Added: 0003673
2012-06-04 20:58	TIHan	Note Edited: 0003673	View Revisions
2012-06-04 21:01	Torr Samaho	Note Added: 0003674
2012-06-04 21:14	TIHan	Note Added: 0003675
2012-06-05 21:15	Torr Samaho	Note Added: 0003677
2012-06-05 21:22	TIHan	Note Added: 0003678
2012-06-05 21:23	TIHan	Note Edited: 0003678	View Revisions
2012-06-05 21:24	TIHan	Note Edited: 0003678	View Revisions
2012-06-05 21:26	TIHan	Note Edited: 0003678	View Revisions
2012-06-06 18:49	Torr Samaho	Note Added: 0003684
2012-06-06 19:12	TIHan	Note Added: 0003685
2012-06-06 19:19	TIHan	Note Edited: 0003685	View Revisions
2012-06-06 23:11	TIHan	Note Edited: 0003685	View Revisions
2012-06-09 13:22	Torr Samaho	Category	General => Bug
2012-06-09 18:39	TIHan	File Added: aprop_test_03_modif.wad
2012-06-09 18:40	TIHan	Note Added: 0003713
2012-06-09 18:40	TIHan	Note Edited: 0003713	View Revisions
2012-06-12 19:27	Torr Samaho	Note Added: 0003742
2012-06-12 23:49	TIHan	Note Added: 0003748
2012-06-12 23:57	TIHan	Note Edited: 0003748	View Revisions
2012-06-14 07:35	ZzZombo	Note Added: 0003762
2012-06-14 14:27	TIHan	Note Added: 0003768
2012-06-14 14:37	ZzZombo	Note Added: 0003769
2012-06-14 14:38	ZzZombo	Note Edited: 0003769	View Revisions
2012-06-14 18:54	Torr Samaho	Note Added: 0003773
2012-06-14 18:55	Torr Samaho	Status	feedback => needs testing
2012-06-14 18:55	Torr Samaho	Additional Information Updated	View Revisions
2012-08-02 01:51	Arcodios	Note Added: 0004185
2012-08-02 05:22	Torr Samaho	Status	needs testing => resolved
2012-08-02 05:23	Torr Samaho	Fixed in Version	=> 1.0
2012-08-02 05:23	Torr Samaho	Resolution	open => fixed
2018-09-30 19:53	Blzut3	Status	resolved => closed

Notes
(0003599) TIHan (reporter) 2012-05-08 05:40	Fixed. 'https://bitbucket.org/TIHan/tst/changeset/8c0fe17ed9b7 [^]' This should also take care of bug 832.

(0003634) Torr Samaho (administrator) 2012-05-14 20:11	p_acs.cpp: I think the spectator checks in DLevelScript::SetActorProperty should be moved into DoSetActorProperty (then you only need the check once). I need to look into the other changes in more detail. At first glance, the abBlendColor change looks very hackish.

(0003635) TIHan (reporter) 2012-05-14 20:43 edited on: 2012-05-14 20:46	> p_acs.cpp: I think the spectator checks in DLevelScript::SetActorProperty should be moved into DoSetActorProperty (then you only need the check once). Will do. > At first glance, the abBlendColor change looks very hackish. I couldn't decide what to do here. Other options for this are: 1. New Command: SERVERCOMMANDS_GivePowerupWithBlendColor - this will set the exact blend color ( the full 4 bytes ) that the powerup is using to clients. 2. Introduce an enum type with all the possible blend color types, and use this to send to clients in SERVERCOMMANDS_GivePowerup, or introduce a new command for this. 3. Use current solution but create SERVERCOMMANDS_GivePowerupWithBlendColor to handle it. Solutions 1 and 2, to me, seem unecessary - as needing to know when the blend color changed on the server only occurs once in the entire codebase AFAIK.

(0003660) Torr Samaho (administrator) 2012-05-27 10:47	I took care of the p_acs.cpp change and added the spectator part of your patch (with an added it->player check). We still need to think about abBlendColor.

(0003665) TIHan (reporter) 2012-06-03 19:53	The question will be is how we will handle BlendColor itself on powerups. So far, the assignments to BlendColor are straight forward and happen in sync with the client and server (no need to tell clients about it). However, this one instance breaks it, so we need a way to tell the client that the BlendColor has been changed.

(0003670) Torr Samaho (administrator) 2012-06-04 19:37	Why don't we do what we always do in such a case then and create a new SVC2 command that just sets the blend color? BTW: Since we can't introduce new commands for every single combination of possible server commands, I think we shouldn't introduce something like SERVERCOMMANDS_GivePowerUpWithBlendColor.

(0003671) TIHan (reporter) 2012-06-04 19:47 edited on: 2012-06-04 19:51	> Why don't we do what we always do in such a case then and create a new SVC2 command that just sets the blend color? IIRC, I don't think there is way to reference the PowerUp from the client if we were to use specifically a new command that sets the blend color of a PowerUp. PowerUps don't have NetIds? Edit: Perhaps we could do it by its class type?

(0003672) Torr Samaho (administrator) 2012-06-04 20:49 edited on: 2012-06-04 20:50	Well, I didn't look at this in detail, but you destroy a PowerUp based on its netId in your patch: if (power != 4) { AInventory *item = it->FindInventory (powers[power]); if (item != NULL) { // [WS] Destroy the powerup. if ( NETWORK_GetState( ) == NETSTATE_SERVER ) SERVERCOMMANDS_DestroyThing( item ); item->Destroy (); } } So this part of your patch either doesn't work or it is how can reference the PowerUp on the client.

(0003673) TIHan (reporter) 2012-06-04 20:57 edited on: 2012-06-04 20:58	> Well, I didn't look at this in detail, but you destroy a PowerUp based on its netId in your patch. Then my patch wouldn't work and the powerup wouldn't actually be destroyed on the client. Looks like we are going to have to come up with IDs for inventory. Edit: I can't look at anything right now. I will later today.

(0003674) Torr Samaho (administrator) 2012-06-04 21:01	Removing inventory items can be done with SERVERCOMMANDS_TakeInventory.

(0003675) TIHan (reporter) 2012-06-04 21:14	Then I guess it does it by inventory type in there. The amount is 0 and destroys it? I question if there would be an edge case when setting the BlendColor by getting the powerup by inventory type.

(0003677) Torr Samaho (administrator) 2012-06-05 21:15	What really worries me right now is the following: If SERVERCOMMANDS_DestroyThing has no effect where you put it in your patch, how did this change pass your testing? BTW: I'm pretty sure that the inventory has a netID on the server. The server just doesn't tell it to the client when giving it.

(0003678) TIHan (reporter) 2012-06-05 21:22 edited on: 2012-06-05 21:26	I think that SERVERCOMMANDS_DestroyThing or Destroy() never got fired in my testing. It would only get fired when you attempt to take away the powerup using a script argument(arg1) of 0. So this wouldn't be related to the original bug, but it was something I noticed and thought should be handled. Sorry for this confusion, as I could have mentioned my reasoning there. So, a question to you, if inventory does have a netID, should we send it over to the client?

(0003684) Torr Samaho (administrator) 2012-06-06 18:49	Oh, then I guess I didn't make clear that I assume all code submissions to be at least very briefly tested. This assumption is so fundamental to me, that I thought this goes without saying. So to the very least I think each code change should be tested at least once to confirm that they have the desired effect at least on one occasion (of course thorough testing would be much better). Putting completely untested code in a stable branch IMHO is a bad idea. This example here clearly shows how easily completely wrong things could get in otherwise. If for some reason you can't test a patch or parts of it but still really want to submit it, please make clear in the submission which parts are untested. > So, a question to you, if inventory does have a netID, should we send it over to the client? Not unless we absolutely have to. Of course sending it needs additional traffic and so far we managed to do everything without it. I guess identifying by name should be fine, or can you have the same item multiple times (instead of just once but with an amount higher than 1)?

(0003685) TIHan (reporter) 2012-06-06 19:12 edited on: 2012-06-06 23:11	Then the whole issue was me not testing parts of my code changes, but only testing the bug itself. Fair enough. I will need to make changes to ACS in this case. Edited again: Actually, it is fair to point out that I didn't test those pieces. I get it. However, I don't need a paragraph lecture about it. I'll correct whatever mistakes I make, and we move on. No sense in lecturing me this way. I will attempt to try it by inventory type and see what occurs.

(0003713) TIHan (reporter) 2012-06-09 18:40 edited on: 2012-06-09 18:40	'https://bitbucket.org/TIHan/zandronum/changesets/tip/branch(%22bug_833%22) [^]' This solves the bug like you suggested. I also uploaded a modified wad that tests specifically if the inventory item gets destroyed and it works properly.

(0003742) Torr Samaho (administrator) 2012-06-12 19:27	The implementation of SERVERCOMMANDS_SetPowerupBlendColor is strange. If ( BlendColor != INVERSECOLOR ) it does absolutely nothing except for eating bandwidth. If you want to add a command that can nothing but set the blend color of a powerup to INVERSECOLOR (which should be sufficient here) it should be named as such and refuse to do anything in case ( BlendColor != INVERSECOLOR ). Unless you plan to implement a list of commonly used blend colors it's also not necessary to send abBlendColor. If you want a command that can more than just setting INVERSECOLOR, instead of a list, sending the actual PalEntry probably would be better though. BTW: It would be very nice if you would commit separate things separately. For instance, the introduction of SERVERCOMMANDS_SetPowerupBlendColor has nothing to do with the NetCommand rewrite of SERVERCOMMANDS_GivePowerup, so having them in independent commits would be beneficial. And why did you remove the "Can we have multiple amounts of a powerup?" comment? Did you find out that one can have multiple amounts of a powerup?

(0003748) TIHan (reporter) 2012-06-12 23:49 edited on: 2012-06-12 23:57	'https://bitbucket.org/TIHan/zandronum/changeset/7a42f5b395ad [^]' This time I'm just sending the full PalEntry over. The check whether to send it or not will need to be outside the server command itself as you see "if ( power == 0 )" in p_lnspec.cpp. Before I had a check inside the server command to see if the blend color was 0, then don't send; however, I thought either at some point we may want to set the blend color to 0 and inform the clients in the future. For now, we only have one use case to use SetPowerupBlendColor, and that is exactly what it does regardless of what it is. Conserving bandwidth will be up to how you call it, rather than the command itself. As far as having multiple powerups of the same type, it looks like you can't. From my understanding, when you receive a powerup of the same type that you already have, it will just reset the effecttics.

(0003762) ZzZombo (reporter) 2012-06-14 07:35	Guys, are you aware of "Using this [SetPlayerProperty] special to grant powerup effects to players has been deprecated. Consider using the GiveInventory function for this purpose instead."? Maybe you just will deprecate it as well?

(0003768) TIHan (reporter) 2012-06-14 14:27	> Maybe you just will deprecate it as well? Even though it is deprecated, it is still used in some wads and we have to handle it.

(0003769) ZzZombo (reporter) 2012-06-14 14:37 edited on: 2012-06-14 14:38	I think that's just pointless. If something is considered obsolete then that means nobody should ever use it. If one continues then he just made his choice and should deal with all side effects, especially if an proper alternative way is given.

(0003773) Torr Samaho (administrator) 2012-06-14 18:54	Thanks! I added the patch with minor modifications ("break" instead of "return" and "PLAYER_IsValidPlayerWithMo" instead of "PLAYER_IsValidPlayer plus mo check").

(0004185) Arcodios (reporter) 2012-08-02 01:51	Fixed.