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

Improve comment about the "ED" tile bug #421

Merged
merged 2 commits into from
Jul 17, 2023
Merged

Conversation

SatoMew
Copy link
Contributor

@SatoMew SatoMew commented Jul 16, 2023

This workaround was added as a comment in commit 8a6d46f.

@dannye
Copy link
Member

dannye commented Jul 16, 2023

Why remove it?

@SatoMew
Copy link
Contributor Author

SatoMew commented Jul 16, 2023

Why remove it?

@Rangi42 said "No comment, no official doc" when I asked her about this type of comment a while back.

@dannye
Copy link
Member

dannye commented Jul 16, 2023

I see no reason to remove the recording of this information. Even ignoring flawed emulators, this information is relevant to anyone who tries to move the graphic to any bank other than bank 0 or bank 1.

If anything, the comment should elaborate even more..

@SatoMew
Copy link
Contributor Author

SatoMew commented Jul 16, 2023

Would a wiki tutorial be a suitable alternative?

@dannye
Copy link
Member

dannye commented Jul 16, 2023

I don't think so. I think inline in source is good.

@Rangi42
Copy link
Member

Rangi42 commented Jul 16, 2023

If you really want this documented, the actual code line should be explicit about why it's "wrong":

    ; BUG: BANK("Home") should be BANK(ED_Tile), although it coincidentally works as-is
    lb bc, BANK("Home"), (ED_TileEnd - ED_Tile) / $8

@SatoMew SatoMew changed the title Remove emulator bug comment Improve comment about the "ED" tile bug Jul 17, 2023
@SatoMew
Copy link
Contributor Author

SatoMew commented Jul 17, 2023

Sounds good! 😃

@dannye
Copy link
Member

dannye commented Jul 17, 2023

Yeah, I like that better 👍

@dannye dannye merged commit a38c792 into pret:master Jul 17, 2023
1 check passed
@SatoMew SatoMew deleted the noemubug branch July 17, 2023 11:09
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