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

Further improve NPC trade labels and comments #493

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

Conversation

SatoMew
Copy link
Contributor

@SatoMew SatoMew commented Feb 1, 2025

This PR partially amends #384 by correcting the information about the previously mentioned localization oversight and reworks the documentation of InGameTrade_CheckForTradeEvo a bit.

Should evolve_trade.asm be renamed to avoid confusion with EVOLVE_TRADE since the file is only for InGameTrade_CheckForTradeEvo?

@Rangi42 Rangi42 requested a review from dannye February 7, 2025 16:24
@Narishma-gb
Copy link
Contributor

Slightly off-topic, would you consider using constants in in_game_trades.asm ?
Trade text dialogs are indexed at wInGameTradeTextPointerTableIndex, and are used as raw numbers in DoInGameTradeDialogue.

; in game trade text pointers
; TradeTextPointersX indexes (see engine/events/in_game_trades.asm)
	const_def
	const TEXT_WANNA_TRADE ; 0
	const TEXT_NO_TRADE    ; 1
	const TEXT_WRONG_MON   ; 2
	const TEXT_THANKS      ; 3
	const TEXT_AFTER_TRADE ; 4

I used these constant names in Pokegreen, see https://github.com/Narishma-gb/pokegreen/blob/24ad50fa9c7295e7b2fd003a7b40386cc02a7d32/engine/events/in_game_trades.asm

@SatoMew SatoMew closed this Feb 8, 2025
@SatoMew SatoMew reopened this Feb 17, 2025
@dannye
Copy link
Member

dannye commented Feb 18, 2025

@SatoMew Can you please provide a bit of info around some of your your actions. For example: Why did you close this? Why did you reopen it? What changed?

Narishma-gb made a correction which you appear to agree with (you even already resolved the thread) but you never pushed any additional changes.

@SatoMew
Copy link
Contributor Author

SatoMew commented Feb 18, 2025

@SatoMew Can you please provide a bit of info around some of your your actions. For example: Why did you close this? Why did you reopen it? What changed?

I felt the need to reconsider. That's all I would like to share.

@SatoMew
Copy link
Contributor Author

SatoMew commented Mar 9, 2025

Any update?

@Narishma-gb
Copy link
Contributor

I like the new explanation. It addresses 2 possible inaccuracies:

; This may have been why Red/Green's trades
; were used instead, where none can evolve.

The opposite is just as likely. After they fixed the routine, they decided to use Red/Green trade set, and left the code untouched because it did not break anything. Some time later came the final names.

; Japanese Red and Green used TRADE_DIALOGSET_CASUAL, and had
; the same species as English Red and Blue

Red/Green also has 3 dialog sets, ranging from casual to more formal.
Since the modified TRADE_DIALOGSET_EVOLUTION was used in 4 in-game trades, two had to switch to CASUAL or HAPPY in Blue, because they did not offer evolving mon.
As a side effect, the sprites were changed from Cook to Youngster, and from Gentleman to LittleGirl, to match the change in speech formality.

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.

None yet

3 participants