Match vulkan_core.h updates

[MarkCallow]

Several formats have been moved into core, most notably the ASTC HDR formats. This PR updates the tests accordingly.

[MarkCallow]

Updating the tests was painful. The .json test files were straightforward. The issue was with all the files, both input and golden' that have the format name in their names. I couldn't leave them as-is because the tests generate the name to look for using the format given in the subcase being run which, in many cases, is also passed to the --format argument.

A particular problem was the ASTC HDR formats being moved to core. The only way to distinguish these from the ASTC 3D formats was by the number of dimensions in the block size in the name. Careful REs were needed. I had to use -E and -regex with find for the needed behaviour.

Another thing that made it harder than necessary is that --regen-golden only takes a single test name.

[aqnuep]

@lexaknyazev, FYI.

@MarkCallow, please allow me to review this and the code changes (KhronosGroup/KTX-Software#783). I should get around to do it sometime next week.

The main reason for you needing to do these renames is because the scripting of KTX-Software for the Vulkan enums does not handle aliases at all.

Personally, I would have preferred to instead first fix the underlying mechanics of the Vulkan enum parsing to also handle that too, instead of this change, which seems to break backward-compatibility, because it now won't accept the EXT suffixes previously accepted.

Also, AFAICT these PRs don't fix the main problem of requiring a private, modified copy of the Vulkan headers, which I think is the main problem we should solve (and would probably require a vk.xml based parser).

Honestly, I think that this is not the right time to do this work, instead we should spend the time to do a proper overhaul and create a future-proof solution that doesn't require a custom maintained private Vulkan header copy.

[MarkCallow]

@MarkCallow, please allow me to review this and the code changes (KhronosGroup/KTX-Software#783). I should get around to do it sometime next week.

Absolutely.

I'd be happy to have the parsing improved. I expect changes to source the info from vk.xml will take some time. Note that switch body generators will have to match enumerator values of aliases and only include one value in the switches.

Also, AFAICT these PRs don't fix the main problem of requiring a private, modified copy of the Vulkan headers, which I think is the main problem we should solve (and would probably require a vk.xml based parser).

The modified copy of the Vulkan headers is a detail. The main problem is the failure to handle the aliases which, I agree, is probably easier to do in a python program using data from vk.xml.

[MarkCallow]

I have updated mkvkformatfiles, which generates stringToVkFormat to include all the _EXT etc. suffixed formats that have core equivalents so ktx create now accepts those formats. I pushed the changes to KhronosGroup/KTX-Software#783.

Only 3 of the ktx create tests have *A4*UNORM subcases: basic_png.json, fewer_components_png.json and encode_error_format.json. I've added subcases to the first 2 to test acceptance of the *A4*UNORM_EXT formats. Adding subcases to the third makes it look for raw input files with *A4*UNORM_EXT. I could easily create these but maybe it is better to modify the test runner so it can be told to look for files without the "_EXT" as those files have identical content. If we don't we will likely eventually end up with a large number of identical but differently named files.

Since ktx create does not accept any ASTC HDR formats there are no tests to augment for those. Internally it uses the enumeration values to get for valid formats so it will reject both of *ASTC_*_SFLOAT_BLOCK{,_EXT} as they have the same value.

The test changes I made in earlier commits (validate, info) remain valid as new undecorated format names are preferred. If I had modified mkvkformatfiles earlier those changes would not strictly have been necessary. The tests would have worked with the deprecated names.

[MarkCallow]

If we don't we will likely eventually end up with a large number of identical but differently named files.

The same applies to golden files. After adding the subcases, there are new golden _EXT files with identical content to the non-_EXT files. I don't know if there is a reasonable solution to avoid the duplication.

[aqnuep]

I could easily create these but maybe it is better to modify the test runner so it can be told to look for files without the "_EXT" as those files have identical content.

The test cases have to explicitly specify the output and reference files to compare. Any sort of "guess work" added to the test runner to look for similarly named files would be a bad idea.

If we don't we will likely eventually end up with a large number of identical but differently named files.

That is true, but also somewhat expected. We anyway have to deal with inputs/outputs with only slight differences which hardly justify the additional size, but anything else would require significantly more complicated test framework and thus higher engineering resourcing needed for creating and maintaining the tests.

If we really want to avoid such duplication, there's always the option to rewrite the test cases in such a way that the format and filename parameters come from pairs of values instead of from a single argument combination list, as it is used in certain test cases, but I think that would also be an overkill compared to just having dedicated files for each format enum.

Let's discuss this topic sometime and come up with a strategy. I'd prefer that over trying to rush in a partial solution.

[aqnuep]

Btw, the changes to the reference results seem reasonable, barring concerns about not testing any more the suffixed versions of the format names.

[MarkCallow]

Let's discuss this topic sometime and come up with a strategy. I'd prefer that over trying to rush in a partial solution.

Thanks for your thoughts. I agree it is best not to complicate the test framework. Let's discuss at some convenient point.

Btw, the changes to the reference results seem reasonable, barring concerns about not testing any more the suffixed versions of the format names.

What places is it failing to test suffixed versions? As far as I can see, other than encode_error_format.json, I've added _EXT tests to the only places that matter. For the validate and info tests, only file names and, for info, some golden files are affected by the removal of the suffixed versions. I will make the duplicate (in content) input files for ``encode_error_format.json` and add the suffixed names to the test. Tomorrow.

[aqnuep]

What places is it failing to test suffixed versions? As far as I can see, other than encode_error_format.json, I've added _EXT tests to the only places that matter. For the validate and info tests, only file names and, for info, some golden files are affected by the removal of the suffixed versions. I will make the duplicate (in content) input files for ``encode_error_format.json` and add the suffixed names to the test. Tomorrow.

I only skimmed through the individual JSON changes, but I just saw you removing the tests with the "_EXT" suffix and replace with suffixless cases, and I was wondering whether we lose regression testing that ensures that the "_EXT" suffixed names are still accepted as the format parameter of the commands.

As long as you're sure we still have cases that do cover those, I'm happy, and thank you for going through this pain. I hope at least you could use bulk renames and search/replaces.

[MarkCallow]

I only skimmed through the individual JSON changes, but I just saw you removing the tests with the "_EXT" suffix and replace with suffixless cases, and I was wondering whether we lose regression testing that ensures that the "_EXT" suffixed names are still accepted as the format parameter of the commands.

Only create has a --format argument and the subcases I added today cover the suffixed versions. For the other tests, e.g. the raw tests, it is just file names that have changed. The code being tested is unaware of the textual format names so there is no reduction in coverage.

[aqnuep]

Only create has a --format argument and the subcases I added today cover the suffixed versions. For the other tests, e.g. the raw tests, it is just file names that have changed. The code being tested is unaware of the textual format names so there is no reduction in coverage.

Thanks, that makes sense, and went back in the meantime to check this, and I see there are at least some tests that include that, which should be sufficient.

[aqnuep]

LGTM.