Final inventory actor pointer

[FDARI]

Preferred patch: http://forum.zdoom.org/viewtopic.php?f=34&t=29364

I keep older versions of this post for reference, since the description gets major rewrites whenever I discover new quirks. We're well beyond bugfixing, and into fine tuning the submission. In this version of the post, I'll simply list the affected functions, related functions and their previous and current behaviour. The most recent change is based on discoveries regarding how player targets are handled in most code other than A_JumpIfInTargetInventory, A_GiveToTarget and A_TakeFromTarget.

Players generally don't use stored pointers. Several existing action functions, and some that I recently added, will get NULL as target when used in a player class.

A_RearrangePointers
No difference since its introduction. The discoveries made do not apply very well to this function. As players don't use stored pointers (much/at all), it would be a best practice not to use this in player classes.

A_GiveInventory, A_TakeInventory, A_TransferPointer
None of these functions were able to get a player's linetarget. A new AAPTR-flag has been added. AAPTR_PLAYER_GETTARGET. This flag can be or'ed with any standard selector (target,master,tracer,default,null) and will override the standard selector if used on a player. (If this flag is used, and a player is not looking at a target, the result will be NULL.)

AAPTR_PLAYER_GETTARGET is the same as AAPTR_PLAYER_GETTARGET|AAPTR_DEFAULT
It will get a player's target, and a regular actor's "self".

AAPTR_PLAYER_GETTARGET|AAPTR_NULL will get a player's target, or null.

AAPTR_PLAYER_GETTARGET|AAPTR_TARGET will get a player's target, or the stored target pointer.
And of course it can be used with AAPTR_MASTER and AAPTR_TRACER as well.

Note: AAPTR_DEFAULT|MASTER|TRACER|SELF and NULL can not be combined; any two of these in a single command would be ambiguous.

Note: Obviously AAPTR_PLAYER_GETTARGET can not be used to indicate a storage location for an actor pointer (relevant to A_TransferPointer and A_RearrangePointers). "GET" has been used in the name to help make this point.

A_GiveToTarget, A_TakeFromTarget, A_JumpIfInTargetInventory - before any changes I made
Do these even work when used in a player class? My reading of the code says they do not, and tests have so far agreed with me. These functions are not changed in that regard, however.

A_GiveToTarget, A_TakeFromTarget
In a previous patch, I wrongfully assumed that the length of a parameter list in zdoom.pk3 would limit the length of a parameter list submitted to an action function. Non-existent parameters would have returned 0, which is the desired value. These functions share code with A_GiveInventory and A_TakeInventory, and this code expects three parameters. If the third parameter is 0, the functions behave normally.

As this is not the case, a default parameter of 0 has been appended to these functions in the pk3.

As a side effect, it is possible to specify a third parameter in these two functions. If used, these parameters will forward the effect from the caller's target to the indicated pointer held by the caller's target. Some might find a use for this, but the parameter should probably not be put to regular use. As A_GiveInventory and A_TakeInventory duplicate and extend this behaviour, it might be time to deprecate the target-specific codepointers.

A_JumpIfInventory, A_JumpIfInTargetInventory
Added the ability to select a pointer for the test, exactly like A_GiveInventory and A_TakeInventory. A_JumpIfInventory was skipped in the code submission that has been added, and thus the A_JumpIfInTargetInventory extra-parameter-bug has not been introduced. Here, A_JumpIfInventory is extended with an extra parameter, which means that A_JumpIfInTargetInventory has received the exact same extension. As is the case for A_GiveToTarget and A_TakeFromTarget, I suggest deprecation of the more limited function.

---

Additions and safeguards for use of actor pointers (Target, Tracer, Master, Default, Null):

A_GiveInventory: Added safeguards in C++
A_TakeInventory: Added safeguards in C++
A_JumpIfInventory: Added ability to jump based on Target, Tracer or Master inventory.

Fixes:
A_GiveToTarget, A_TakeFromTarget, A_JumpIfInTargetInventory: These code pointers were initially assumed to send 0 (AAPTR_DEFAULT) automatically if the final parameter was not added to zdoom.pk3. This turns out not to be the case. Therefore, the pointer-parameter has been added also to these functions.

Considerations regarding this fix:

It is required for full compatibility.
It enables an unintended, but otherwise stable feature, that should possibly not be recommended for use: Using the "target"-variants of inventory functions and forwarding the check/operation from target to target->master|target|tracer. If this possibility is a problem, I can make a slightly less elegant solution in the C++ code instead.

EDIT: New fixes, or a slight difference from original functions
A_GiveToTarget, A_TakeFromTarget and A_JumpIfInTargetInventory all use the "self->target" pointer indiscriminately. If the caller is a player, target seems to be unassigned most or all of the time. Several code pointers use a different method to acquire the target if the caller is a player. I have implemented this behaviour for GiveInventory, TakeInventory and JumpIfInventory. When AAPTR_TARGET is used (affect/check target inventory) and the caller is a player, the linetarget method is used to get a target pointer. This introduces an actual difference from A_GiveToTarget etc. as these would most likely exit with a NULL target.


Now the diff-patch contains changes to wadsrc/static.

---

The recently added modifications to A_GiveInventory and A_TakeInventory use a trick which is also readily available for A_JumpIfInventory, where the flexibility in designing for A_...Target... can be used to apply any pointer. As the feature has been requested, I submit further modified code for letting A_JumpIfInventory work with any of your three pointers.

This submission also contains some slight changes in the code I added last time: The logic was not as robust as I would have wanted it.

The code I wrote would never be called with a null pointer for original actor. Not "needing" a null-check for my code given that it is called only in the ways I have enabled, I put this (preexisting) verification after the code that gets the actor's pointer data. In this patch, I have moved null-checking to the top, so that my code will return quietly instead of crashing if somebody manages to both a execute it with both a pointer flag and a null actor (that cannot be dereferenced for the requested pointer).

If the pointer is changed to null by my code, the function returns, just as if the pointer had already been null. This applies to DoGiveInventory, DoTakeInventory, and (NEW) DoJumpIfInventory.

I have no example of this feature being useful, but it has been requested, and it is a very easy implementation.

PK3-changes:
actors\actor.txt, line 199: action native A_JumpIfInventory(class<Inventory> itemtype, int itemamount, state label, int owner = AAPTR_DEFAULT);

EDIT:
There is a possibility that further change to this patch is required or desired. Please download and run it in tests; use it for your purposes. When I am again, and rightly, satisfied that this is ready, I'll post the message.

Change to patch: Fewer safeguards were required, as Target is not affected directly by A_CopyFriendliness
Change to patch: "while(true)"... is replaced by "for (;;)"...

---

Parameters not defined in zdoom.pk3 are not necessarily null/0. Therefore added parameters need suitable default values in ZDOOM.PK3 also where there parameter was not originally meant to be used. This causes an unplanned expansion of the feature.

Modified lines 240 - 242 of my ZDOOM.PK3:

Code: Select all

action native A_JumpIfInTargetInventory(class<Inventory> itemtype, int amount, state label, int forward_ptr = AAPTR_DEFAULT);
action native A_GiveToTarget(class<Inventory> itemtype, int amount = 0, int forward_ptr = AAPTR_DEFAULT);
action native A_TakeFromTarget(class<Inventory> itemtype, int amount = 0, int flags = 0, int forward_ptr = AAPTR_DEFAULT);

Example of A_JumpIfInventory() with AAPTR_TRACER in use: Strife dagger with backstabbing. Sorry about publishing a resource that's not functional in any acknowledged version of the engine; I didn't notice at first.

EDIT: Attachments removed; they have served their purpose.

[Major Cooke]

While I'm definitely for this, unfortunately this breaks the Stone of Temperance in AEoD whose main function is to stop seeking. I don't know what's the cause of it for certain, but I've flipped back and forth between this patch and the first one.

I'm making a rather uneducated guess but it might be the part where you put res=true instead of false...

[FDARI]

Silence, noone must ever know I did that!
Uploading fixed patch now. Too much copy paste in the first one. (How did I ever expect to get away with copy-paste?)

[Major Cooke]

I just saw you post this so I'm giving it a shot now.

[Major Cooke]

Okay, for some reason, it still likes to break the Stone of Temperance in AEoD and I don't know why. However, thinking about it, I can't help but think that we could do a better job of redesigning how it acts ourselves... Hmm...

[FDARI]

If my code changes the behaviour of something, then my code is not correct. It should be 100% compatible with previous releases. Is the stone of temperance sufficiently independent that decorate + description would allow me to examine the effect? I would like to find out how on earth I broke it.

Edit: Do you know what code pointers are broken? I keep thinking it must be one of the old targetinventory-related functions (Give, Take, JumpIf). What else is there? I considered the possibility that I'd have broken it by not adding a 3. parameter to these functions, as they share parameter interpretation code with the functions I extended. However, I am rather confident that missing parameters return 0, which is the only value I want from them anyway.

[Major Cooke]

Well, here's the stone of temperance to kinda give you an idea...

Code: Select all

ACTOR StoneOfTemperance : CustomInventory
{
    +FLOATBOB
    +COUNTITEM
    +INVENTORY.FANCYPICKUPSOUND
    +INVENTORY.INVBAR
    Scale 0.25
    Inventory.Amount 1
    Inventory.MaxAmount 10
    Inventory.PickupMessage "You found the Stone of Temperance!"
    Inventory.PickupSound "powerup/voidstone"
    Inventory.UseSound "powerup/stone/use"
    Inventory.Icon "HARTVOST"
    States
    {
    Spawn:
	VOST ABCDEFGHIJKLMNOPQRSTUVWX 2
	Loop
    Use:
	TNT1 A 0 A_JumpIfInventory("NoSeekTimer",1,4)
	TNT1 A 0 A_GiveInventory("NoSeekTimer",1)
	TNT1 A 0 A_GiveInventory("NoSeekPalette",1)
	TNT1 A 0 A_SpawnItemEx("NoSeek1",0,0,0,0,0,0,0,32,0)
	Stop
	TNT1 A 0
	Stop
    }
}

ACTOR PowerNoSeek : PowerProtection
{
    DamageFactor "Normal", 0.666
    Inventory.Icon "ARTVOIDA"
}

ACTOR NoSeekPalette : PowerupGiver
{
    -INVENTORY.INVBAR
    +INVENTORY.INTERHUBSTRIP
    Inventory.MaxAmount 1
    Powerup.Type "NoSeek"
    Powerup.Color Magenta4 0.2
    Powerup.Duration 1575
    +AUTOACTIVATE
}

ACTOR NoSeekTimer : CustomInventory
{
   +NOTIMEFREEZE
   +INVENTORY.INTERHUBSTRIP
   States
   {
   Use:
       TNT1 A 0
       Stop
   }
}

ACTOR NoSeekTimer2 : Inventory
{
    +NOTIMEFREEZE
    Inventory.Amount 1
    Inventory.MaxAmount 1575
    States
    {
    Spawn:
        TNT1 A 0
        Stop
    }
}

ACTOR NoSeek1
{
    Radius 0
    Height 0
    +NOTIMEFREEZE
    +NOFEAR
    States
    {
    Spawn:
        TNT1 A 0
        TNT1 A 0 A_SpawnItemEx("NoSeekGiver",0,0,0,0,0,0,0,32,0)
        Stop
    }
}

ACTOR NoSeek2 : CustomInventory
{
    +AUTOACTIVATE
    +NOTIMEFREEZE
    States
    {
    Pickup:
        TNT1 A 0 A_ChangeFlag("CANTSEEK",1)
	TNT1 A 0 A_ChangeFlag("NORADIUSDMG",1)
	TNT1 A 0 A_ChangeFlag("DONTRIP",1)
        Stop
    }
}

ACTOR NoSeek3 : CustomInventory
{
    +AUTOACTIVATE
    +NOTIMEFREEZE
    States
    {
    Pickup:
        TNT1 A 0 A_ChangeFlag("CANTSEEK",0)
	TNT1 A 0 A_ChangeFlag("NORADIUSDMG",0)
	TNT1 A 0 A_ChangeFlag("DONTRIP",0)
        Stop
    }
}

ACTOR NoSeekGiver
{
    Health 1000000000
    Radius 0
    Height 0
    Speed 0
    +NONSHOOTABLE
    +MISSILEMORE
    +MISSILEEVENMORE
    +NOTIMEFREEZE
    +NOFEAR
    MinMissileChance 0
    States
    {
    Spawn:
        TNT1 A 1 A_Look
        Loop
    See:
        TNT1 A 1 A_Chase
        Loop
    Missile:
	TNT1 A 0 A_JumpIfInTargetInventory("NoSeekTimer",1,2)
	TNT1 A 0 A_Jump(256,"Death")
	TNT1 A 0 A_GiveToTarget("NoSeek2",1)
	TNT1 A 0 A_JumpIfInventory("NoSeekTimer2",1575,"Death")
	TNT1 A 1 A_GiveInventory("NoSeekTimer2",1)
	Goto Missile+3
    Death:
        TNT1 A 0 A_GiveToTarget("NoSeek3",1)
	TNT1 A 0 A_TakeFromTarget("NoSeekTimer",1)
        Stop
    }
}

It's supposed to do 3 things:
-Make the user unseekable
-Make the user unrippable
-Make the user immune to explosion damage without the +FORCERADIUSDMG flag.

[FDARI]

Well. What I find is that I made an incorrect assumption: The argument list submitted to my action function will not necessarily be limited to the number of arguments specified in ZDOOM.PK3; it may contain unpredictable data beyond this point. That means that the old and true "target" variants of functions will need a slight touch-up to be compatible.

On the other hand: Convoluted and complex code, man! I think it could already have been done in simpler ways, and now you might have more ways to do it. I'll just have to fix us another patch.

[FDARI]

There are two approaches:
1 - Adding logic to the 3 inventory-related functions that checks if the third parameter should be used
2 - Adding the extra parameter in the ZDOOM.PK3 definition, for all the "target" functions. This has the side effect of allowing a two-step jump, while preserving default behaviour. (A_JumpIfInTargetInventory("Item",1,2 AAPTR_MASTER) will only jump if you have a target with a master that has the required amount of the specified item)

Solution 2 is technically most satsifying. But the "target"-functions gain a feature as a side-effect (which we must then decide if should be used or advised against/undocumented). The ability to forward a request to any of the target's pointers like this is slightly inappropriate for the function name, and also inappropriate because the more generic functions I made are meant to overlap completely with target-specific functions.

Solution 1 looks like slightly more engine-work, but may make more sense in some regard. I'm looking for advice / ideas here, before posting a patch.

EDIT: Patch lines for PK3 are added to the first post. It is too easy not to at least put forth the solution.

[Major Cooke]

On the other hand: Convoluted and complex code, man! I think it could already have been done in simpler ways, and now you might have more ways to do it. I'll just have to fix us another patch.

Don't blame me! I didn't make it. I'm just posting it here for convenience. Anyway I'm reading the rest of your posts now...

[Major Cooke]

This patch happened to fix more crashes. Thanks man! Still doesn't fix the issues with the likes of Stone of Temperance though.

Question, how would you go about re-writing the stone of temperance?

[FDARI]

I only used the radiusdamage to test the condition of the stone of temperance, but on my computer the official release and my latest patch worked out the same (as if there was 100% compatibility, which is what I'm aiming for). What remaining differences do you find? Also; if my patch were 100% backward compatible, it should probably not fix any crashes that occured in previous versions. Thus, I'm still wondering what I might be doing wrong. If I can find out how my code affects your crashes, I can probably also find out how to fix them in the first place (without engine update). And I can find any weaknesses in my work.

I don't know exactly how I'd rewrite the stone of temperance, but after reading the source-code I have the impression that: Any/most non-monster actors spawned without SXF_TRANSFERPOINTERS will target their creator like missiles do. This appears to apply to your helper actors (NoSeek1, NoSeekGiver). This probably means that NoSeek1 is immediately set with the necessary target pointer and requires no further preparation; it can give inventory to the target, and perform the work of "NoSeekGiver"; and it does not have to look or chase at all. (Target is correct from the get-go.)

If this is not automatically the case, you can use A_CustomMissile instead, and make it target that way. There might be even simpler solutions, but I don't know the reasoning behind every line of decorate.

[Major Cooke]

Yeah, I was asking for your opinion about it first before jumping onto changing it off the bat.

[Major Cooke]

After some more testing... CustomInventory is no longer performing A_ChangeFlag on the user and I have no idea why. I tested it with A_SetInvulnerable and that worked just fine, but A_ChangeFlag is being absolutely stubborn! That is, using your patch.

Here, try this code:

Code: Select all

Actor TestInv : CustomInventory
{
	States
	{
	Pickup:
		TNT1 A 0 A_SetInvulnerable
		TNT1 A 0 A_ChangeFlag("CANTSEEK",1)
		TNT1 A 0 A_ChangeFlag("NORADIUSDMG",1)
		TNT1 A 0 A_ChangeFlag("DONTRIP",1)
		Stop
	}
}

"Give testinv" in the console and watch. You're invulnerable but you still dont have the flag settings.

[FDARI]

There must be a difference between our versions at this point. I tested that actor. All code pointers were executed with "DoomPlayer" as "self", and all flag changes were made to the DoomPlayer. I also replaced A_SetInvulnerable with A_ChangeFlag("NODAMAGE",1), and it made the player properly invulnerable.

Perhaps I have again failed to post a valid diff after all; my grasp of the svn-tool is far more tenuous than my grasp of C++ and the ZDoom engine (and even that isn't outright amazing yet).

I don't see anything wrong about my patch. I uploaded my thingdef_codeptr - source, which contains all modifications for this patch. Can you test with this code, or use this code to create your own diff-patch and compare with mine? If you get a different patch, then I might have done something wrong when generating mine.

EDIT: PS - Preserve your current version! (I'm sure you will, but then again, is anything certain?)