C targeting API, comments needed

[LoneFox]

How we do target selection and validation in C?

There are three steps in targeting:
1. Determine if any legal targets are available. Done in EVENT_CAN_CAST or EVENT_CAN_ACTIVATE
2. Select targets. Done in EVENT_CAST_SPELL or EVENT_ACTIVATE.
3. Validate targets before resolving the spell or ability. Done in EVENT_RESOLVE_SPELL or EVENT_RESOLVE_ACTIVATION.
For triggered abilities all three happen in EVENT_RESOLVE_TRIGGER.

(NOTE: spells that can target only players skip step 1 and assume that players can always be targeted. Bad news if someone wanted to implement Ivory Mask)

In the exe there are wrapper functions for all three steps and various target types. They all(?) call one big function with different parameters. These wrappers cannot be used directly from C, because they use non-standard calling convention that C compilers don't understand.

My current approach (implemented in functions/targets.asm and used in cards/red_cip_abilities.c) is to use asm functions that take address of a function in the exe as parameter and call that address with stack and registers set up like the function expects. This works, but:

• Some of the functions in exe need extra parameters, such as required color or minimum power. They need to be handled differently.
• I already cheated in naming the constants used to identify functions (TA_CREATURE instead of TARGET_AVAILABLE_CREATURE)
• For some target types there are multiple functions: one for hostile effects (removal), one for beneficial effects (pump), and one for neutral effects. This means multiple versions of the constants (TA_CREATURE_HOSTILE vs. TA_CREATURE_BENEFICIAL) are needed.
• Fixing the cancel bug makes things even worse. We can copy-paste the functions from exe and change the parameter that prevents cancelling, but then we end up with constant names like TA_ARTIFACT_OR_CREATURE_OR_LAND_HOSTILE_NOCANCEL. This is comparable to hello.jpg in ugliness.

We don't want to use the big targeting function directly either. It is too clunky with its kilometer-long parameter list.

One possibility would be defining a structure that stores target definition:

Code: Select all

/* "Untap target blue creature" */
target_definition_t td;
set_default_target_parameters(&td); /* Copy-paste default values programmatically */
td.type = TYPE_CREATURE;
td.color = CARD_COLOR_BLUE; /* COLOR_BLUE is already reserved for mana color... */
td.preferred_controller = player;
if(target_available(player, card, &td)) /* Calls the big targeting function with params from the struct */
  return 1;


So, any better ideas for this? Getting it right on first try is important, because so many cards will use it. Fixing tens or perhaps hundreds of cards to use better API afterwards won't be fun.

[jatill]

I think your suggestion is the best approach. Also, for the more common targeting funcs, like target creature, I'm a fan of creating wrapper functions that do all the annoying work for me.

[jatill]

I don't want to force my functions on you, but be aware that I wrote affect_me(player, card), which is short for (affected_card_controller == player) && (affected_card == card). This is going to come up is SO many cards...

[Snacko]

I like the struct approach, but can't COLOR_* be reused ? Are you sure the function uses different constants to indicate the same colors ?

[LoneFox]

I like the struct approach, but can't COLOR_* be reused ? Are you sure the function uses different constants to indicate the same colors ?

Mana colors are plain numbers (0 = colorless, 1 = black, 2 = blue...). Card colors are bit flags (0x1 = colorless, 0x2 = black, 0x4 = blue...). It's this way because cards can have more than one color.

EDIT: card types are bit flags too, which means C implementation of has_creature_type() still has a bug.

Code: Select all

card_d.type == TYPE_CREATURE

will miss artifact creatures.

[jatill]

LoneFox-
I was just looking over your new functions, and had a couple questions.

1) Do you have any objection to keeping each card in its own .c file? I'm thinking this will be easier to find them in the future.
2) For functions like do_dialog that have multiple unknown params, what do you think about creating do_dialog_abbr that just defaults the unknown params? I know in java you can have multipe functions with the same name, as long as their arg list is different. Can you do that in C also?
3) What does MARK_DONE(instance); do?

[LoneFox]

1) Do you have any objection to keeping each card in its own .c file? I'm thinking this will be easier to find them in the future.

• What is the maximum command line length on Windows and how soon we will hit it?
• Lots of files in one directory means it is hard to find anything from there. Is there a way to make the compiled object files go in their own subdirectory without listing each one separately in the makefile?
• Having multiple similar cards in same file makes it possible to write static functions that are shared between these cards without bloating manalink.h. There is already example of this in red_cip_abilities.c

2) For functions like do_dialog that have multiple unknown params, what do you think about creating do_dialog_abbr that just defaults the unknown params? I know in java you can have multipe functions with the same name, as long as their arg list is different. Can you do that in C also?

Maybe use a macro?

Code: Select all

#define DO_DIALOG(player1, player2, card, text, ai_choice) do_dialog((player1), (player2), (card), -1, -1, (text), (ai_choice))

3) What does MARK_DONE(instance); do?

Code: Select all

mov byte [esi + 0x36], 0

I don't know what this is and why it is needed, just that existing cards with CIP abilities do it.

[Snacko]

On computers running Microsoft Windows XP or later, the maximum length of the string that you can use at the command prompt is 8191 characters. On computers running Microsoft Windows 2000 or Windows NT 4.0, the maximum length of the string that you can use at the command prompt is 2047 characters.

Currently at ~2k so the limit isn't so far. I don't know maybe gcc has an option to read those from a file ? If not then we really need to group the cards by something like color/?.

Code: Select all

card_d.type == TYPE_CREATURE

My bad was fixing this, but with the merfolk example it slipped by. It's just a one liner fix

Code: Select all

card_d.type & TYPE_CREATURE

so I suppose you can include it, because I don't feel like posting another package and making more confusion.
I also broke

Code: Select all

STATE_TAPPED =            1<<1,

should be

Code: Select all

STATE_TAPPED =            1<<4,

Don't know how that happened ...


The macro could be replaced by a inline function, either way works.

The make file has a regex build in you can easily make a bin folder and throw object files in a separate folder.

Also I think we should check for shroud as I'm not sure those underlying functions do or don't.

NOTE: spells that can target only players skip step 1 and assume that players can always be targeted. Bad news if someone wanted to implement Ivory Mask

Rewrite all cards and check for player shroud ? Lot's of work but doable in the long run.

[jatill]

Currently at ~2k so the limit isn't so far. I don't know maybe gcc has an option to read those from a file ? If not then we really need to group the cards by something like color/?.

How about just cards.c and funcs.c, then? I just want to be able to find the specific code for a card without hunting (or grepping). I never considered the command line limit. Seems silly

Also I think we should check for shroud as I'm not sure those underlying functions do or don't....

They must, since no old cards were re-implemented when shroud was added. We can probably find a similar fix if player shroud is added. No need dealing with the edge cases until they actually exist, I think.

[jatill]

I think the difference between 401A40 and 401A80 is that the first one doesn't return esi if the card isn't in play. So how would I update the c-code to test is a card is in play? (the ASm does a test [esi+6f], x80)

[LoneFox]

How about just cards.c and funcs.c, then? I just want to be able to find the specific code for a card without hunting (or grepping). I never considered the command line limit. Seems silly

All in one file grows too large quickly. Is it simple enough for you if I use color and card type for grouping? Like white_creatures.c, white_enchantments.c, and white_oneshot.c (instants and sorceries tend to share a lot of code, so I think it's bad idea to separate them).

Another thing:

Code: Select all

int has_mana_multi(int player, int colorless, int black, int blue, int green, int red, int white){
    MANA_COLORLESS =   colorless;
    MANA_BLACK =      black;
    MANA_BLUE =      blue;
    MANA_GREEN =      green;
    MANA_RED =         red;
    MANA_WHITE =      white;
    return check_mana_multi(player);
}

void charge_mana_multi(int player, int colorless, int black, int blue, int green, int red, int white){
    PAY_MANA_COLORLESS =   colorless;
    PAY_MANA_BLACK =      black;
    PAY_MANA_BLUE =      blue;
    PAY_MANA_GREEN =      green;
    PAY_MANA_RED =         red;
    PAY_MANA_WHITE =      white;
    charge_mana(player, 0, 0);
}


Why all these MANA_* and PAY_MANA_* ? Wouldn't it be better to use arrays? Also, ALL_UPPERCASE is normally used for constants only.

[jatill]

All in one file grows too large quickly. Is it simple enough for you if I use color and card type for grouping? Like white_creatures.c, white_enchantments.c, and white_oneshot.c (instants and sorceries tend to share a lot of code, so I think it's bad idea to separate them).

This seems fine as long as functions are easily sharable across all spell types. For example, the hybrid sub works for spells and creatures. I'm kind of just following the examples put in by you and Snacko, since I don't know much a C conventions.

Why all these MANA_* and PAY_MANA_* ? Wouldn't it be better to use arrays? Also, ALL_UPPERCASE is normally used for constants only.

Again, just mimicing what I saw. If you want to clean this stuff to your stylistic preferences, I'm cool with that.

[jatill]

While you're poking around, maybe you can answer another question I have.
Take the code I wrote for survival of the fittest, uncomment the commented line, and change the 71 to 0x71 or EVENT_RESOLVE_SPELL. Survival works, but it gets a trashed registers error. It's the commented out lione that does it. Any idea why that error would happen? The same thing is called for thirst for knowledge and that doesn't trash anything.

[LoneFox]

This seems fine as long as functions are easily sharable across all spell types. For example, the hybrid sub works for spells and creatures. I'm kind of just following the examples put in by you and Snacko, since I don't know much a C conventions.

Of course. The functions directory is still available for such functions.
I think hybrids can be put all in one file regardless of type, because there are few of them compared to single-color or normal multicolor cards.

Take the code I wrote for survival of the fittest, uncomment the commented line, and change the 71 to 0x71 or EVENT_RESOLVE_SPELL. Survival works, but it gets a trashed registers error. It's the commented out lione that does it. Any idea why that error would happen? The same thing is called for thirst for knowledge and that doesn't trash anything.

Instruction at 0x4E78FA destroys EDI. Thirst of Knowledge works because its code saves and restores EDI.
Interestingly, I spotted this same bug yesterday in different place, namely the function used by Hunted Wumpus and Elvish Piper. This is why professional programmers hate copy-pasting code. Bugs reproduce fast enough on their own, we don't need to help them .

[jatill]

Thanks so much for hunting that down. I fixed the instruction, so survival works great now.