An Implementation of Group-scoped Variables

Implementation for local group variables. Group variables are scoped local to the group (Core, Alt 1, Alt 2, etc) they are in and not useable in other groups.

Group variables are updated in the Pause pass. Set GroupVar conditions that occur before an active pause are processed; those after an active pause are not processed. This is a deliberate design decision to allow some control over when a variable is updated, but also to ensure that any memrefs used by the functional logic of a condition get updated before evaluation.

Group variables used as offsets to indirection are implemented as indirect memrefs. A group variable maintains references to all indirect memrefs they affect and, when updated by a Set Group Var condition the variable will update the address field of all memrefs. The implementation and memory allocation of this array of memrefs is understood to likely not be correct based on how memory is allocated in the library and may fail for cases where it needs to be serialized for things like save states (currently this serialization is not [knowingly] implemented). Guidance on proper design here is desired.

i and r were chosen as prefixes for global variable operands. i for integer and r for real, with GroupVar being used as a type.

Set Group Var has the source on the left and the variable to be assigned on the right. This uses the equals operator (any other shall be an error), but we could implement a specialty operator for clarity (such as >> - similar to C++'s std::cin >> myVariable).

Index/Number/ID of group variable is arbitrary to the developer. If the developer uses Group Variables 5, 17, and 35552, then only those three group variables will be created. If a user uses GroupVar 0x00 in core and also uses GroupVar 0x00 in Alt1, they will be different group variables.

Naming is open to discussion as 'Set Group Var' condition, 'Group Var' 'Mem Group Var/GroupVar(Mem)/GroupVar as Mem', etc may be unwieldly in presentation.

This feature is intended solve these problems, but may have further use cases:

Need for more complex mathematics (a variable can be used as for intermediate steps).
Need for a calculated offset from a pointer target (asked for previously as an "Add Index" condition), which is always the case when the index and data are both behind pointers.
Need a way to use pointers whose location in RAM is calculated (no root pointer/chain that leads to them) without needing to scan large data sets.
Comparison of memory values where one value is behind a pointer and the other is not (without having to use subtraction and worry about underflow).

If implemented, recommended structure for logic in a group will be as follows. This ought to be mentioned in developer docs:

Set GroupVar logic that should always occur (even if group is paused)
Pauses
Set GroupVar logic that should not occur if group is paused
Other logic.

Example use case for retrieving and using boss pointer and boss arena object data in Metroid Prime's Flaahgra Boss Fight:

Example 1: Calculate offset in object table of boss pointer using Boss's UID, then add it to the table pointer to calculate an 'effective address' of the boss pointer, to be captured in a GroupVar

Add Source   Mem      16-Bit BE  BossUID     & 0x3FF
Set GroupVar Value               0x0         = GroupVar 0x00 //GV0 == BossUID & 0x3ff
Add Source   GroupVar            0x00        * Value    0x08       
Set GroupVar Value               0x04        = GroupVar 0x00 //GV0 == (BossUID & 0x3ff)*8 + 4
Add Source   Mem      32-Bit BE  ObjTablePtr
Set GroupVar GroupVar            0x00        = GroupVar 0x00 //GV0 == ObjTablePtr+(BossUID & 0x3ff)*8 + 4

Add Address  GroupVar            0x00
Set GroupVar Mem      32-Bit BE  0x00        = GroupVar 0x01 //GV1 == Pointer to Boss Data

Example 2: Grab the UID of Flaahgra Mirror Object 1 from behind the boss pointer and use it to calculate the offset in the object table of the pointer to the mirror, then use that to get the pointer of the mirror and store in group variable 2:

Add Address  GroupVar            0x01
Set GroupVar Mem      16-Bit BE  OfstToM1UID = GroupVar 0x00 //here GV0 is now the UID of Mirror 1
Add Source   GroupVar            0x00        & 0x3FF
Set GroupVar Value               0x0         = GroupVar 0x00 //GV0 == MirrorUID & 0x3ff
Add Source   GroupVar            0x00        * Value    0x08        
Set GroupVar Value               0x04        = GroupVar 0x00 //GV0 == (MirrorUID & 0x3ff)*8 + 4
Add Source   Mem      32-Bit BE  ObjTablePtr
Set GroupVar GroupVar            0x00        = GroupVar 0x00 //GV0 == ObjTablePtr+(MirrorUID & 0x3ff)*8 + 4

Add Address  GroupVar            0x00
Set GroupVar Mem      32-Bit BE  0x00        = GroupVar 0x02 //GV2 == Pointer to Mirror 1 Data

Repeat similar logic for other three mirrors to store in GV3-5.

I really wish you'd bring up your ideas for discussion before doing any implementation.

For full transparency, I haven't looked at the code changes at all - just your example.

I feel like this is a simplified version of variables (https://github.com/RetroAchievements/RAIntegration/discussions/780), and that would be the better direction to take things. However, that's a much larger project, requiring changes at all levels - DB/API/DLL. You're effectively defining variables that only live within a group. However, the way you're capturing and using multiple variables is hard to understand, and doesn't really translate well to the full variable support I envision.

The general concept of chaining operations on a value is sound, and probably should be implemented separately from variables. I believe that would be an acceptable direction to take this, but anything beyond a single accumulator would be better handled by the variables implementation.

Given your example:

Add Source   Mem      16-Bit BE BossUID     & 0x3FF
Set GroupVar Value              0x0         = GroupVar 0x00 //GV0 == BossUID & 0x3ff
Add Source   GroupVar           0x00        * Value    0x08       
Set GroupVar Value              0x04        = GroupVar 0x00 //GV0 == (BossUID & 0x3ff)*8 + 4

With just a single accumulator, it becomes:

SetAccumulator Mem      16-Bit BE BossUID     & Value 0x3FF // accumulator = (BossUID & 0x3ff)
SetAccumulator $accumulator                   * Value 0x08  // accumulator = (BossUID & 0x3FF) * 8
SetAccumulator $accumulator                   + Value 0x04  // accumulator = (BossUID & 0x3FF) * 8 + 4

And you can keep chaining it for your logic:

SetAccumulator $accumulator + Mem      32-Bit BE  ObjTablePtr // accumulator = ObjTablePtr + (BossUID & 0x3FF) * 8 + 4

Add Address  $accumulator
SetAccumulator Mem      16-Bit BE  OfstToM1UID & Value 0x3FF // accumulator = (MirrorUID & 0x3FF)
SetAccumulator $accumulator                    * Value 0x08  // accumulator = (MirrorUID & 0x3FF) * 8
SetAccumulator $accumulator                    + Value 0x04  // accumulator = (MirrorUID & 0x3FF) * 8 + 4
SetAccumulator $accumulator + Mem     32-Bit BE  ObjTablePtr // accumulator = ObjTablePtr + (MirrorUID & 0x3FF) * 8 + 4

Add Address $accumulator
            Mem 16-BitBE OfstToData = Value Target // compare 16-bit value at ($accumulator + OfstToData) to Target

If we expand that in the direction that I'd like to do for variables, that all becomes a single variable:

SetAccumulator Mem      16-Bit BE BossUID     & Value 0x3FF // accumulator = (BossUID & 0x3ff)
SetAccumulator $accumulator                   * Value 0x08  // accumulator = (BossUID & 0x3FF) * 8
SetAccumulator $accumulator                   + Value 0x04  // accumulator = (BossUID & 0x3FF) * 8 + 4
SetAccumulator $accumulator + Mem      32-Bit BE  ObjTablePtr // accumulator = ObjTablePtr + (BossUID & 0x3FF) * 8 + 4
Add Address  $accumulator
SetAccumulator Mem      16-Bit BE  OfstToM1UID & Value 0x3FF // accumulator = (MirrorUID & 0x3FF)
SetAccumulator $accumulator                    * Value 0x08  // accumulator = (MirrorUID & 0x3FF) * 8
SetAccumulator $accumulator                    + Value 0x04  // accumulator = (MirrorUID & 0x3FF) * 8 + 4
SetAccumulator $accumulator + Mem   32-Bit BE  ObjTablePtr // accumulator = ObjTablePtr + (MirrorUID & 0x3FF) * 8 + 4
Add Address    $accumulator
Measured       Mem 16-BitBE OfstToData // capture 16-bit value at ($accumulator + OfstToData)

Which can then be used like a memory accessor:

            Mem  $data = Value Target
            Prev $data < Value Target

I'd be fine implementing this as a single accumulator. The biggest hurdle is going to be how to select it as a variable when constructing the achievement. And I'd like to design it in such a way that it would transition nicely to full variable support in the future. In my examples, I've effectively labelled it the accumulator as a variable with the hard-coded name "accumulator".

Given the existing UI for editing achievements, we could make Accumulator a Size (which you were doing with the GroupVar, but the Value field would be unused - instead of a GroupVar index):

AddAddress Accumulator

However, for better compatibility with variables, it should just be a variable:

AddAddress Varaible $accumulator

And the Value field would be a dropdown populated with all known variables, which would only be the $accumulator at this point.

My examples above also assume the ability to directly apply addition, which would be an improvement over having to use AddSource:

AddSource      Value 0x04
SetAccumulator $accumulator   // accumulator = (BossUID & 0x3FF) * 8 + 4

I learn better by doing. This was me trying to see how this might work and ending up at this draft PR because I was pleased with the results. It is about the third iteration of an earlier idea, but it's a draft for a reason. I'm not afraid to throw out work. Just wanting this to be a point of discussion with some meat behind it. I will try to do more initial discussion in the future.

I agree my approach here is akin to a simpler form of https://github.com/RetroAchievements/RAIntegration/discussions/780.

I see the following benefits:

Not as large an undertaking - ought to be quicker to implement and solves needs that exist currently (not just mine--I just use mine as examples). I am confident it can be refined into code that is maintainable.
Fits with the existing asset editor grid
Uses existing RA logic patterns to set and consume these 'variables'. I think this should be relatively understandable for new and old devs.
Can capture multiple values
Each captured value can be used in multiple places.
Ought to be able coexist with a more extensive variable framework as its own creature (as a narrower use case. I'll argue this below)

The main contra:

Some amount of use case overlap with a grander variable system design.

I argue that my implementation (or something similar) would still have a use case as something temporary to capture a value for reuse, created at the point where it is needed; since it may not need a name nor the support of something at a higher level of scope nor long-term persistence. Good for cases to avoid having to more formally define a few temporary variables to accomplish some task. I also see it as a lightweight option that can immediately benefit the ecosystem.

Note/Correction: I've implemented GroupVar as a type. ~Size was still needed for a GroupVar being used as an offset for indirection) and because of this also added a Mem/Delta/BCD/etc variant as a type for use as an offset. I'm admittedly not particularly keen on that, but it seemed a reasonable means to accomplish the task, with standard GroupVar more akin to a 'Value' constant in usage and the others more akin to a memref in usage.~

Using Add Source was the design goal for using the existing toolkit as a means to set things. Follow-ups re-used/updated the same variable as a consequence of this. I don't personally find it too confusing; I see the same kind of step-by-step updates with students new to programming all the time.

I do think GroupVar is a bit of an unexciting name. It has been at varying points 'TempVar' and 'Local Group Var' and others. GroupVar seemed to at least capture the essence of what it is: temporary data storage in a condition group for re-use.

My concern with the accumulator approach is that in my example I will be needing the boss pointer and the four mirror pointers multiple times. Do I have to redo the calculation every time I need one of those? What if I need both in the same comparison (probably not in my use case, but I am certain it will come up). It also doesn't seem like it'd fit into the existing presentation of the asset editor grid.

Arguably, my approach is a set of user-defined (by number/ID) accumulators.

Here's a further example for something I would definitely do (in addition to also checking boss HP and a few other related things, also being the boss pointer):

Add Address GroupVar            0x01 //Boss Pointer
And Next    Mem      32-Bit BE  OfstToBossState   = Value AttackMode
Add Address GroupVar            0x02 //Mirror1 Pointer
And Next    Delta    32-Bit BE  OfstToMirrorState = Value Flipped
Add Address GroupVar            0x02 //Mirror1 Pointer
Reset If    Mem      32-Bit BE  OfstToMirrorState = Value NotFlipped

Add Address GroupVar            0x01 //Boss Pointer
And Next    Mem      32-Bit BE  OfstToBossState   = Value AttackMode
Add Address GroupVar            0x03 //Mirror2 Pointer
And Next    Delta    32-Bit BE  OfstToMirrorState = Value Flipped
Add Address GroupVar            0x03 //Mirror2 Pointer
Reset If    Mem      32-Bit BE  OfstToMirrorState = Value NotFlipped

etc

Another use I'd have in the example game that I didn't describe would be actually detecting events (they are not simple bitflags). Memory relays are in a vector behind a two-pointer chain, so the first value is the count of items in the vector, and the rest are the IDs of events (32-Bit BE). When this index changes (Mem > Delta), I'd really like to be able to use that index as indirection to read the ID of the item at the end of the vector. This would let me edge-check the start (and completion) of a TON of events in the game and open up a large variety of achievements that are currently impossible.

Another newer developer has wanted the ability to calculate the distance between two points for use in a comparison. I don't believe that can be done with a single accumulator (and also it would at least need exponentiation added for the squares and square root) and would instead need more than one accumulator and/or stack-based math.

I do like your idea for variables, but it is ambitious and feels like a long way off. I put my own take for something of similar scope awhile back here: https://github.com/RetroAchievements/RAIntegration/discussions/1072 (instead of adding to 780 for some reason) and while I no longer like that particular idea, it does additionally explain my current needs.

I am concerned that we are in a 'perfect-as-the-enemy-of-good' situation though.

I am open to adapting this idea and willing to learn whatever I need in order to get this or some other alternative into an acceptable, maintainable state that meets use-case needs. What I would like to avoid if possible is releasing one of the higher-anticipated GameCube sets in Metroid Prime (and Prime 2), while having to say "Here's the set. It's fairly basic, but we'll add the cool stuff in a revision when the toolkit allows it." Let's catch the tool kit up. I'll gladly help with anything that solves the stated use cases in the top comment and any other potential needs that crop up over time.

Were I to continue this work I would add additional tests (for runtime progress write/read, some other cases), beyond what is currently done. Some little refactoring is likely to be done, plus anything that would shake out on review/feedback. Edit: I refactored the group var to memref interaction to avoid the memory leaks I had initially, as a learning exercise at least.

@Jamiras I re-read your accumulator examples further. I am inclined to think this is also a potential solution, if there can be multiple accumulators. The biggest issue I see here is how do you specify which accumulator you are setting? Having a SetAccumulator1, Set Accumulator2, ..., SetAccumulatorN conditions seems a bit much. For reasons why I am asking, see my resets example in my last comment. Since SetAccumulator would break the And Next chain (or would it? But that feels like it'd be a non-obvious case. I can make a discussion for that, I suppose, but that's my primary concern with that approach. I do see that since we could add the calculated offset onto the value of the pointer before using it in the add address, using such a 'group var' or accumulator value as an offset isn't needed and probably simplifies the presentation/usage.

I would still appreciate a look over the implementation I did here even if it's not ultimately an acceptable direction, just to be able to have an errors in logic, usage, etc pointed out so as to avoid them in future efforts. Could set this to ready for review to facilitate that?

~Hold off. I want to simply things and then discuss everything further first.~

RetroAchievements / rcheevos

An Implementation of Group-scoped Variables #339