An Implementation of Group-scoped Variables #339
Conversation
… group vars. To test and debug.
…ty of setting a variable followed by measuring it.
… as the offset to a pointer.
…st for pause + set group variable interaction.
|
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 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: 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 (RetroAchievements/RAIntegration#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: With just a single accumulator, it becomes: And you can keep chaining it for your logic: If we expand that in the direction that I'd like to do for variables, that all becomes a single variable: Which can then be used like a memory accessor: 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): However, for better compatibility with variables, it should just be a variable: 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: |
…P direction should this work continue.
|
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 RetroAchievements/RAIntegration#780. I see the following benefits:
The main contra:
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. 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): 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: RetroAchievements/RAIntegration#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? |
|
Started discussion for Accumulators here: RetroAchievements/RAIntegration#1097 |
|
|
… the pointer+offset resulting address can be captured and used on the Add Address line.
|
Is there still something here worth pursuing? Or is it covered by Remember/Recall? The only thing I'm seeing is the desire for multiple remembered values, but you discounted that in #1097:
|
Having used the feature a fair bit now, I did run into a situation where multiple remembered values would have been handy: And another case where it would have been nice to directly remember both the pointer chain to the WRAM and to the SRAM separately in the NES Metroid subset of Metroid Prime 1. Neither was a necessity, but would be helpful. Future variables could handle it (but if you only need them for one achievement, it feels cumbersome and harder to follow if you have to create separate assets) A junior recently had a big complicated math problem that sorta worked out thanks to multiple alt values (it was for Rich Presence), but was nearly an impossibility. So more complicated math would be another use case though I have a separate idea (Stack Math) that I haven't proposed yet because I've been trying to reign in claims/various team responsibilities. I'm carrying on a bit. I think this PR has some merit in its ideas still, but the implemented code would need to be reworked if this idea were to be added, so it can probably be closed and I can keep the branch for reference if needed. If this were to be implemented, I think we could just leverage Remember for setting these 'variables' (perhaps a different name than GroupVar would be warranted to avoid confusion): For anything that isn't going to be re-used across achievements/groups, I could see a benefit to something like this. |
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:
If implemented, recommended structure for logic in a group will be as follows. This ought to be mentioned in developer docs: