Rewrite RGBGFX in C++ #981
Conversation
ISSOtm
added
enhancement
|
Oh yeah; it also currently doesn't build due to a lot of issues with the mixed C/C++ builds. This'll obviously need fixing, but it does compile on at least my machine. It also compiles on at least one CI configuration, but the disasms fail since it's currently incomplete. |
src/gfx/convert.cpp
Outdated
|
|
||
| class ImagePalette { | ||
| // Use as many slots as there are CGB colors (plus transparency) | ||
| std::array<std::optional<Rgba>, 0x8001> _colors; |
There was a problem hiding this comment.
| std::array<std::optional<Rgba>, 0x8001> _colors; | |
| std::array<std::optional<Rgba>, 1 << (5 * 3) + 1> _colors; |
There was a problem hiding this comment.
I agree on the usefulness of this change, but as-is, the calculation seems arbitrary at first glance, and has confused me at first. Constants would help, but I will hold off until some of them are (potentially) introduced by further modifications.
There was a problem hiding this comment.
Given that this appears in a couple other files, I still think it's worth a constant; or is it still too early to bother?
There was a problem hiding this comment.
I'm not sure what to name that constant, tbh.
|
|
||
| // All efficiencies are identical iff min equals max | ||
| // TODO: maybe not ideal to re-compute these two? | ||
| // TODO: yikes for float comparison! I *think* this threshold is OK? |
There was a problem hiding this comment.
Let's say protoPalettes[maxEfficiencyIter->palIndex] is A and protoPalettes[minEfficiencyIter->palIndex] is B. This compares efficiency(A) and efficiency(B) with a threshold. But efficiency(x) = x.size() / bestPal.relSizeOf(x); so instead, how about finding the LCM of bestPal.relSizeOf(A) and bestPal.relSizeOf(B) and comparing A.size() and B.size() based on that?
There was a problem hiding this comment.
Can you explain that a bit more?
There was a problem hiding this comment.
A := protoPalettes[maxEfficiencyIter->protoPalIndex]
B := protoPalettes[minEfficiencyIter->protoPalIndex]
condition := efficiency(A) ==~ efficiency(B)
condition := A.size() / bestPal.relSizeOf(A) ==~ B.size() / bestPal.relSizeOf(B) // float threshold
condition := A.size() * bestPal.relSizeOf(B) == B.size() * bestPal.relSizeOf(A) // exact comparison
There was a problem hiding this comment.
Ah, I meant regarding how the two algorithms are equivalent, sorry
There was a problem hiding this comment.
I just multiplied both sides by the denominators.
There was a problem hiding this comment.
auto efficiency = [&bestPal](ProtoPalette const &pal) {
return pal.size() / bestPal.relSizeOf(pal);
};There was a problem hiding this comment.
Oh, right. Forgive me for lacking in confidence, but I'd like to implement this change after we've set up a test corpus, so we can ensure that this does not accidentally changes generation output.
There was a problem hiding this comment.
Ah, this doesn't work because relSizeOf is floating-point as well. Any other ideas?
ISSOtm
force-pushed
the
rgbgfx-cxx
branch
2 times, most recently
from
2ddac3b
to
75d54f4
Compare
These offsets should only be applied to a tile ID read as input... but this ain't one!
No such tests yet, but the infrastructure will be there.
This is not intended to test libpng as much as checking that we behave correctly if libpng gives us an error
Simplifies iterating over tiles and attributes at the same time
Should improve performance. This version is cooler, and also does not suffer from iteration limits
Less duplication = good
As it turns out, it is really difficult to implement, and can be dealt with later.
Also correct minor blunders in the man page
|
As it turns out, |
Currently missing from the old version:
-f("fixing" the input image to be indexed)-m(the code for detecting mirrored tiles is missing, but all of the "plumbing" is otherwise there)-C-d(removed)-x(though I need to check the exact functionality the old one has)Also the man page is still a draft and needs to be fleshed outIt only needs proofreading now (and-U's description when that is implemented)More planned features are not implemented yet either:
Quantization?Some things may also be bugged:
rgbgfx#618)...and performance remains to be checked.
We need to set up some tests, honestly.