zzorba / marvelsdb

MarvelsDB - a deck builders for Marvel Champions - The Card Game
49 stars 28 forks source link

Add support for attack_text, thwart_text, and defense_text for heroes and allies #207

Closed jordanweiler closed 9 months ago

jordanweiler commented 10 months ago

The new Psylocke hero card (https://github.com/zzorba/marvelsdb-json-data/pull/395) has a star next to her attack, thwart, and defense with the same interrupt text for all three.

attack_text is already a column for the card table but it's currently just used for villains/minions. The thwart_text and defense_text columns didn't exist so I'm adding those to the database. I'm adding support for all three properties for hero cards and just attack_text and thwart_text for ally cards.

ALTER TABLE card ADD thwart_text LONGTEXT DEFAULT NULL;
ALTER TABLE card ADD defense_text LONGTEXT DEFAULT NULL;

In app.format.ts I created a new replaceSpecialCharactersInText function to reduce code duplication for getting the traits, icons, and newlines formatted in the text properties. I reused this for all the text related properties (text, attack_text, thwart_text, defense_text, scheme_text, boost_text, back_text). I also deleted format.html_page because I couldn't find any references to it so it seemed like it wasn't used.

Screen Shot 2023-08-29 at 10 09 02 PM Screen Shot 2023-08-29 at 10 08 49 PM

This should fix https://github.com/zzorba/marvelsdb/issues/199.

Kamalisk commented 10 months ago

So I think ultimately the attack/thwart text field was a mistake in the first place. It is all just text on the card. What this means is that if you try to search for card text you need to search all the fields that are considered the card "text".

Boost text was the start of it all, which I think really could just be part of the text of the card.

I am not sure there is any practical reason to have them as seperate fields.

jordanweiler commented 10 months ago

So I think ultimately the attack/thwart text field was a mistake in the first place. It is all just text on the card. What this means is that if you try to search for card text you need to search all the fields that are considered the card "text".

Boost text was the start of it all, which I think really could just be part of the text of the card.

I am not sure there is any practical reason to have them as seperate fields.

That would simplify things to just keep the text field and not have separate variations for all the other properties. The current way definitely doesn't scale well.

Just keeping the text field means there's not an easy way to tag attack/thwart/defense/scheme/etc with a star to know there's text associated with that action. Is that something you think should be supported? Maybe it's fine and not worth 100% matching the real cards, dunno.

It also means that some cards still currently support this model (villains and minions supporting attack_text and scheme_text) so is your long term plan to remove that type of functionality to get everything consistently using just text?

Kamalisk commented 10 months ago

I am not really sure why anyone would need to search for cards that have stars on them, but searching for "star" or "boost" would cover most cases.

My thought was to do so eventually yeah, needs some effort to migrate the json files and the translations, but I had been fine leaving the text in the regular text field on newer cards.

I think if we want to keep track of it having a star, we can change the fields to boolean fields, eg. boost_attack: true, which can let us show the icons in the card display.

jordanweiler commented 10 months ago

I think if we want to keep track of it having a star, we can change the fields to boolean fields, eg. boost_attack: true, which can let us show the icons in the card display.

This would mean that we'd need columns for each field that can have a star/asterisk associated with it. I think this makes sense and separates the text of the card with the star icon next to certain fields. I think we might want to name them {field}_star to be really generic since sometimes it's for a boost but sometimes it's an interrupt/response. Boost seems specific to the encounter cards. I think the boolean column list would be close to this:

Does this sound like a reasonable direction?

Kamalisk commented 9 months ago

that is the best direction, and keeps the card text in the card text field, which is better for translations and searching. So cards with that can be added without, and added in later once it supports the boolean fields.

jordanweiler commented 9 months ago

Sounds good. I'll try to work on that over the next few weeks.