Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add merged placeholder text for trainer name with class #5622

Open
wants to merge 2 commits into
base: upcoming
Choose a base branch
from

Conversation

kittenchilly
Copy link

@kittenchilly kittenchilly commented Oct 30, 2024

Description

Adds 4 new merged placeholders from text that combine both the trainer name and the trainer class.
This allows dynamic new lines to break without being awkward when handling trainer names. It also fixes some issues with spaces if the string is used by both the player and NPCs.

Images

image

Discord contact info

kittenchilly

Comment on lines +3542 to +3545
StringCopy(text, BattleStringGetOpponentClassByTrainerId(gTrainerBattleOpponent_A));
StringAppend(text, gText_Space2);
StringAppend(text, BattleStringGetOpponentNameByTrainerId(gTrainerBattleOpponent_A, text, multiplayerId, GetBattlerAtPosition(B_POSITION_OPPONENT_LEFT)));
toCpy = text;
Copy link
Collaborator

@mrgriffin mrgriffin Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter too much, but I think this would be more efficient if written like:

Suggested change
StringCopy(text, BattleStringGetOpponentClassByTrainerId(gTrainerBattleOpponent_A));
StringAppend(text, gText_Space2);
StringAppend(text, BattleStringGetOpponentNameByTrainerId(gTrainerBattleOpponent_A, text, multiplayerId, GetBattlerAtPosition(B_POSITION_OPPONENT_LEFT)));
toCpy = text;
toCpy = text;
text = StringCopy(text, BattleStringGetOpponentClassByTrainerId(gTrainerBattleOpponent_A));
text = StringAppend(text, gText_Space2);
text = StringAppend(text, BattleStringGetOpponentNameByTrainerId(gTrainerBattleOpponent_A, text, multiplayerId, GetBattlerAtPosition(B_POSITION_OPPONENT_LEFT)));

As-is, StringAppend has to scan all of text until it finds the EOS character. The change here is that StringCopy and StringAppend return a pointer to the EOS, so no scanning is necessary.

(toCpy = text; then gets moved to the first thing we do because we're overwriting text)

EDIT: Technically StringCopy and StringAppend are the same thing when *text points at an EOS, but I don't know I'd go so far as to just use StringCopy. (It would be ever so slightly faster)

@Bassoonian
Copy link
Collaborator

I would personally try to avoid adding more and more placeholders, which feels unsustainable in the long run. Figuring out a no-break space would probably be more useful in the long run (e.g. in dialogue or other messages)

@kittenchilly
Copy link
Author

kittenchilly commented Oct 30, 2024

I would personally try to avoid adding more and more placeholders, which feels unsustainable in the long run. Figuring out a no-break space would probably be more useful in the long run (e.g. in dialogue or other messages)

This isn't just for the dynamic line break, its also for strings that both NPCs (have a trainer class) and the player (doesn't have a trainer class) use. I don't think I would've done a placeholder if that wasn't a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants