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.

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