Skip to content

Conversation

@partyvan
Copy link

@partyvan partyvan commented Sep 9, 2025

ok so in the process of fitting distance stuff into ImageConversion i became quite wary of the many reinterpret_cast<float *> kicking around, and sure enough, a small test program:

#include <vtfpp/vtfpp.h>

int
main(int argc, char *argv[])
{
	auto vtf = vtfpp::VTF::create(argv[1], vtfpp::VTF::CreationOptions{.outputFormat = vtfpp::ImageFormat::RGBA8888});
	auto rfmt = vtf.getFormat();
	auto rwidth = vtf.getWidth();
	auto rheight = vtf.getHeight();
	auto datas = vtf.getImageDataAs(rfmt);
	auto floaties = vtfpp::ImageConversion::convertImageDataToFormat(datas, rfmt, vtfpp::ImageFormat::RGBA32323232F, rwidth, rheight);
	auto opts = vtfpp::VTF::CreationOptions {.outputFormat = vtfpp::ImageFormat::RGBA32323232F};
	auto floatytex = vtfpp::VTF::create(floaties, vtfpp::ImageFormat::RGBA32323232F, rwidth, rheight, opts);
	auto success = floatytex.bake(std::endian::native == std::endian::big ? "from_be.vtf" : "from_le.vtf");

	return !success;
}

resulted in a nonsense vtf full of nans on my powerpc box. at time of writing, 33ebaec represents the absolute minimum it took to roundtrip correctly from img->tex on a big-endian machine to tex->img on a little-endian machine, by just invoking bufferstream's templates with enough information for its own byte order stuff to kick in.

question: in your opinion, is this something worth caring about? ot1h, who's running sourcepp on a BE machine in 2025 besides me for shits and giggles; otoh, it is technically billed as a general-purpose library, and depending on host byte order feels icky in such a setting. if it is something worth caring about, i could go up and down making sure any float/u16/etc reinterpretations do the appropriate shift/mask song and dance routine (ime, compilers are smart enough to emit equivalent assembly to reinterpret_cast on LE targets, and put registers in LE mode on bi-endian targets) so intermediate structures are always pc vtf output order by default, and chuck down some appropriate unit tests; if not, that's cool too.

Copy link
Owner

@craftablescience craftablescience left a comment

Choose a reason for hiding this comment

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

It would be really nice if this library worked properly on big-endian devices like consoles/quirky computers, not much of a concern for me because I have so few big-endian devices (one, PS3) but yes I will take any fixes you deign to implement, thanks!

@partyvan
Copy link
Author

partyvan commented Sep 10, 2025

here we go, force pushed a rough example of what doing it at storage level would look like. can confirm that this produces correct primary texture data on either architecture (thumbnails and a few other things are still inconsistent), at the expense of exposing users to some wacky wrapper types. at a cursory glance it takes clang 19 -O2 to transform operator= into a single mov on intel across all instantiations, while gcc 14 needs -fpeel-loops on top of -O2 in the case of 32-bit underlying types. msvc seems to struggle to determine the loop bounds no matter what though, so might need to hand-unroll it.

@craftablescience
Copy link
Owner

I have some tests to visually test basic sdk2013 formats, L155 of test/vtfpp.cpp to enable. Might help test all of the formats look right

@partyvan
Copy link
Author

partyvan commented Sep 12, 2025

steady progress: entire vtfpp suite (incl extended) ostensibly passes on ppc & s390x; anything that depends on bitfields is still off visually

edit: ok yeah upon further examination, the combination of (byte order portability) and (memory representation matching disk representation by default) simply is not accommodated by bitfields. so continuing to make this library portable would require a breaking api change concerning the pixel format structs (rip cute .r .g .b member access) whether the new underlying implementation be shifting and masking onto an appropriately-sized scalar type, or std::bitset, or whatever. even the classic "ifdef the fields in reverse order" trick is technically ub in c++ despite c allowing it, and even if it weren't, you'd no longer be able to use aggregate init without explicit member names.

RFC, but barring any objections i'll do up a fresh range of v2 pixel formats, with uniform constexpr callable accessors, explicit constructors of initializer list, etc, use those under the hood, and declspec/attr deprecated the old ones.

@partyvan
Copy link
Author

partyvan commented Sep 14, 2025

ok! ImagePixelV2 is in place and used in enough places to produce visually correct results on BE targets for all uncompressed pixel formats, incl. the bit-packed ones. after sorting out compressonator calls and remaining uses of ImagePixel, vtfpp will be mostly portable.

set(CMAKE_CXX_STANDARD 20)
if(MSVC)
# secret second flag to *actually* make msvc a c++20-compilant compiler
add_compile_options($<$<COMPILE_LANGUAGE:CXX>:/Zc:preprocessor>)
Copy link
Owner

Choose a reason for hiding this comment

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

cmake/AddPrettyParser.cmake does this too, is this necessary to pull to a larger scope?

Copy link
Author

Choose a reason for hiding this comment

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

i don't think it was applying in all the places ImageConversion.h was getting included, but i can double check

@craftablescience
Copy link
Owner

Holy Moly

@partyvan
Copy link
Author

ok ImagePixel is now dead code internally and marked deprecated for external use, and anything pertaining to pixel formats should be portable. ostensibly. i gotta chuck down some more involved tests to make sure nothing got overcorrected.

@partyvan
Copy link
Author

threw in some simple tests to make sure images parse properly and sure enough,

/mnt/git/sourcepp/test/vtfpp.cpp:238: Failure
Expected: (static_cast<PIXFMT::CHANTYPE>(pixels[i].r())) < (static_cast<PIXFMT::CHANTYPE>(pixels[i+1].r())), actual: 63476 vs 25852

[  FAILED  ] vtfpp.read_extfmt_RGBA16161616_png (127 ms)
...
[ RUN      ] vtfpp.read_extfmt_RGB888_qoi
h
/mnt/git/sourcepp/test/vtfpp.cpp:237: Failure
Expected equality of these values:
  vtf.getFormat()
    Which is: 4-byte object <00-00 00-21>
  ImageFormat::RGB888
    Which is: 4-byte object <00-00 00-02>

/mnt/git/sourcepp/test/vtfpp.cpp:237: Failure
Expected equality of these values:
  vtf.getWidth()
    Which is: 0
  32

/mnt/git/sourcepp/test/vtfpp.cpp:237: Failure
Expected equality of these values:
  vtf.getHeight()
    Which is: 0
  32

Segmentation fault (core dumped)

significant work remains on BE :V

at least the happy path from stock-standard 8 bit rgba png to just about any vtf pxfmt is perfect now

@partyvan partyvan changed the title vtfpp: output byte order fixup vtfpp: byte order portability Sep 15, 2025
@partyvan
Copy link
Author

partyvan commented Sep 18, 2025

after much messing about, it seems compressonator has weirder and subtler byte order dependencies than can be solved with a simple 32-bit swizzle before/after, when compressing. makes sense since those formats love crossing byte boundaries so much. genuinely a miracle that decompression is flawless. i am fully on board with your wish to nuke compressonator and will write an according reimplementation of appropriate formats, but it's beyond the scope of this pr.

at the very least, none of the test cases segfault or throw under ppc64 or s390x. mips64 is boned but lol lmao. and a bunch of useful bonus test cases!!

also, since i started, the test image for ARGB8888 has looked the wrong way round and the associated conversion case seems to reflect a typo, but i wasn't sure if it was accounting for some engine quirk where ARGB8888 isn't treated as what it says it's treated as, or something. if not i can fix that too

anyway here's wonderwall

@partyvan partyvan marked this pull request as ready for review September 18, 2025 08:52
@craftablescience
Copy link
Owner

Hot damn this is a lot thank you! I'll try to review over the weekend

Also I'm pretty sure that format looks weird because I used "known good" VTFEdit to generate those back when I used Windows and I think that format might be busted in vtflib or something

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.

2 participants