Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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.

Sign up for GitHub

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

Jump to bottom

Fix RGBGFX reversal #1425

Merged
merged 7 commits into from
Aug 8, 2024
Merged

Fix RGBGFX reversal #1425

merged 7 commits into from
Aug 8, 2024

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Jul 25, 2024 •
edited
Loading

Fixes #1420 (but probably has more left to do)

$ ./rgbgfx -r 40 -t assets/bg_currents.bin -b 1 -o assets/tileset.2bpp assets/map.png
FATAL: Tilemap references tile #192 at (6, 42), but the limit is 188
Conversion aborted after 1 error

@Rangi42 Rangi42 added bug Unexpected behavior / crashes; to be fixed ASAP! rgbgfx This affects RGBGFX labels Jul 25, 2024
@Rangi42 Rangi42 added this to the v0.8.1 milestone Jul 25, 2024
@Rangi42 Rangi42 requested a review from ISSOtm July 25, 2024 20:09
ISSOtm
ISSOtm reviewed Jul 25, 2024
View reviewed changes
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Here are some of the bugs I've spotted from my previous reading of this code...

I think you will understand why I found the task quite daunting. :[

src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
@Rangi42 Rangi42 marked this pull request as draft July 26, 2024 18:00
@Rangi42 Rangi42 marked this pull request as ready for review July 30, 2024 13:37
ISSOtm
ISSOtm reviewed Jul 30, 2024
View reviewed changes
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
@Rangi42 Rangi42 force-pushed the reverse-bugs branch 2 times, most recently from 7805821 to ba0e351 Compare August 3, 2024 02:38
@Rangi42
Copy link
Contributor Author

Rangi42 commented Aug 5, 2024

@ISSOtm Would you mind just pushing directly to this to get it resolved?

@ISSOtm
Copy link
Member

ISSOtm commented Aug 7, 2024 •
edited
Loading

Just rebased. cc @Rangi42!

Just need to add test cases now... which also involves modifying test/gfx/test.sh 😭

@ISSOtm
Properly wrap tile IDs when reversing
000fa32 
I *hate* integer promotion and implicit int casts.
Rangi42
Rangi42 commented Aug 7, 2024
View reviewed changes
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
src/gfx/reverse.cpp Outdated Show resolved Hide resolved
@ISSOtm @Rangi42
Make some minor style fixups
3f9382d 
Co-authored-by: Sylvie <35663410+Rangi42@users.noreply.github.com>
@ISSOtm
Copy link
Member

ISSOtm commented Aug 8, 2024

@Rangi42 is dealing with adding tests, including more structural changes; that'll be done in another PR.

@ISSOtm ISSOtm merged commit 0cd79c3 into gbdev:master Aug 8, 2024
22 checks passed
@Rangi42 Rangi42 deleted the reverse-bugs branch August 8, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ISSOtm ISSOtm ISSOtm approved these changes

Assignees
No one assigned
Labels
bug Unexpected behavior / crashes; to be fixed ASAP! rgbgfx This affects RGBGFX
Projects
None yet
Milestone
v0.9.0
Development

Successfully merging this pull request may close these issues.

RGBGFX does not validate tile indices
2 participants
@Rangi42 @ISSOtm