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

Implement --input-tileset #1464

Merged
merged 12 commits into from
Sep 4, 2024
Merged

Implement --input-tileset #1464

merged 12 commits into from
Sep 4, 2024

Conversation

ISSOtm
Copy link
Member

@ISSOtm ISSOtm commented Aug 10, 2024

Fixes #1485

In particular, a lot of the less clear interactions with other flags (e.g. the lack of -u, how should deduplication be performed, if at all?) have been made into errors, so that we can give them some useful behaviour later on, without fear of breaking backwards compat.

@ISSOtm ISSOtm requested a review from Rangi42 August 10, 2024 18:38
@ISSOtm ISSOtm added enhancement Typically new features; lesser priority than bugs rgbgfx This affects RGBGFX labels Aug 10, 2024
@ISSOtm ISSOtm added this to the v0.9.0 milestone Aug 10, 2024
contrib/bash_compl/_rgbgfx.bash Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
src/gfx/process.cpp Outdated Show resolved Hide resolved
src/gfx/process.cpp Show resolved Hide resolved
src/gfx/process.cpp Show resolved Hide resolved
@ISSOtm ISSOtm requested a review from Rangi42 August 14, 2024 15:50
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
@Rangi42
Copy link
Contributor

Rangi42 commented Sep 3, 2024

Thinking about how --input-tileset interacts with the -o output made me realize: what happens if you don't also pass -u? When you do pass -u, the output image can be understood as a tileset, needing a tilemap to get the output back, But when you don't pass -u, the output image can be understood as a copy of the input image, just encoded as 2BPP instead of PNG. So, if you pass an input tileset, what gets output for these various cases:

  1. Input tile A occurs once in input image
  2. Input tile B occurs three times in input image
  3. Input tile C does not occur in input image
  4. Input tiles D and E are identical, and occur {zero / one / more} times in input image
  5. Input tile F is fully alpha-transparent

(We'll also have to consider "what if an input tile is solid #FF00FF and you pass --background #FF00FF".)

@Rangi42
Copy link
Contributor

Rangi42 commented Sep 3, 2024

Furthermore, regardless of how we handle the above concerns re: not passing -u, we'll need to update the -u docs. It says:

Note that if this option is enabled, no guarantee is made on the order in which tiles are output; while it should be consistent across identical runs of a given rgbgfx release, the same is not true for different releases.

But in fact --input-tileset is making a guarantee ("these tiles will be first"). Do/should we also guarantee that those input tiles will have some specific order among themselves? (Presumably "the same order as they came in, top to bottom left to right, minus any deduplicated ones"?)

@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 3, 2024

Thinking about how --input-tileset interacts with the -o output made me realize: what happens if you don't also pass -u? When you do pass -u, the output image can be understood as a tileset, needing a tilemap to get the output back, But when you don't pass -u, the output image can be understood as a copy of the input image, just encoded as 2BPP instead of PNG. So, if you pass an input tileset, what gets output for these various cases:

1. Input tile A occurs once in input image

2. Input tile B occurs three times in input image

3. Input tile C does not occur in input image

4. Input tiles D and E are identical, and occur {zero / one / more} times in input image

5. Input tile F is fully alpha-transparent

IMO, it's simple: we generate tile data appending it to the input tileset, then run the deduplication pass if requested. This preserves all guarantees, and is a simple yet reasonable model. (Note that there is an explicit check that the deduplication pass does not dedup any of the input tileset's tiles, which would violate the assumption that all input tiles are present as-is in the output.)

The answer to all the following is “emitted once” with -u, obviously, but without:

  1. It's emitted twice.
  2. It's emitted all four times.
  3. It's emitted once.
  4. They're emitted once each.
  5. Made moot by the point below.

(We'll also have to consider "what if an input tile is solid #FF00FF and you pass --background #FF00FF".)

We don't, because deduplication is performed on tile data, so colours are already out of the picture by then. (This is why -c is strongly advised for multi-palette images).

@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 3, 2024

But in fact --input-tileset is making a guarantee ("these tiles will be first"). Do/should we also guarantee that those input tiles will have some specific order among themselves? (Presumably "the same order as they came in, top to bottom left to right, minus any deduplicated ones"?)

We can simply mention that these guarantees are upheld regardless of any dedup flags, or update all four of those flags' descriptions to mention that they only apply to generated tiles but that they are still compared against input tiles. My vote goes for the former.

And, yes, the intent is to basically guarantee that rgbgfx -i in.2bpp img.png -o out.2bpp; truncate -s $(wc -c <in.2bpp) out.2bpp; cmp in.2bpp out.2bpp always returns true. I guess the docs aren't making the full promise, but I'm not sure as to how to word it.

@Rangi42
Copy link
Contributor

Rangi42 commented Sep 3, 2024

Great! That sounds simple, consistent, and predictable. And people almost certainly want -u when using a tileset anyway, so it doesn't have to somehow be useful too. :P

@Rangi42
Copy link
Contributor

Rangi42 commented Sep 3, 2024

the intent is to basically guarantee that rgbgfx -i in.2bpp img.png -o out.2bpp; truncate -s $(wc -c <in.2bpp) out.2bpp; cmp in.2bpp out.2bpp always returns true. I guess the docs aren't making the full promise, but I'm not sure as to how to word it.

Let's get the rest working first and then come back to how to word that; I may have suggestions in a subsequent review.

@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 3, 2024

In theory we could reject -i without -u, as

rgbgfx a.png -o a.2bpp
rgbgfx b.png -o b.2bpp -i a.2bpp

is in fact equivalent to

rgbgfx a.png -o a.2bpp
rgbgfx b.png -o tmp.2bpp
cat a.2bpp tmp.2bpp >b.2bpp

...so we could reserve that combination for later to be more useful behaviour. What do you think?

@Rangi42
Copy link
Contributor

Rangi42 commented Sep 3, 2024

Yes, disallowing -i without -u (at least in its initial release) sounds better.

Rangi42
Rangi42 previously requested changes Sep 4, 2024
man/rgbgfx.1 Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
src/gfx/main.cpp Outdated Show resolved Hide resolved
Rangi42
Rangi42 previously requested changes Sep 4, 2024
man/rgbgfx.1 Outdated Show resolved Hide resolved
man/rgbgfx.1 Outdated Show resolved Hide resolved
test/gfx/input_tileset.flags Show resolved Hide resolved
@Rangi42 Rangi42 merged commit 80d37f9 into gbdev:master Sep 4, 2024
22 checks passed
@ISSOtm ISSOtm deleted the tileset branch September 18, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgbgfx This affects RGBGFX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Specify a pre-existing tileset to use when converting PNG graphics
2 participants