Skip to content

Crusade Against Stupid Switch Statements: Part 0 of N #1325

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

Open
VictorSohier wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Crusade Against Stupid Switch Statements: Part 0 of N #1325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f846c5a
Make image conversion an 2N problem instead of an N^2 problem
VictorSohier Jul 24, 2026
94e9058
pre-commit
VictorSohier Jul 24, 2026
4fc2ec0
Fix code-rabbit suggestions
VictorSohier Jul 24, 2026
e2b46f6
More coderabbit fixes
VictorSohier Jul 24, 2026
5800a27
Fix all the issues and pass all the tests
VictorSohier Jul 25, 2026
a113fb0
Some things code rabbit is actually right on
VictorSohier Jul 25, 2026
9abde09
To satisfy CI
VictorSohier Jul 25, 2026
9012201
Because windows is a :sillybastard:
VictorSohier Jul 25, 2026
bca4038
Because windows is a :sillybastard: part 2
VictorSohier Jul 25, 2026
823fae2
Add templated interface around typeless internals
VictorSohier Jul 26, 2026
06672ec
code rabbit fixes
VictorSohier Aug 30, 2026
57f308d
Compile fixes (Fuck you C++)
VictorSohier Aug 30, 2026
fe3cfb8
Handle Clanker Rabbit's complaints
VictorSohier Aug 30, 2026
de65d1e
Merge branch 'master' into scalar-image-readers-and-writers
VictorSohier Aug 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
294 changes: 65 additions & 229 deletions core/io/image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
#include "core/config/project_settings.h"
#include "core/error/error_macros.h"
#include "core/io/image_loader.h"
#include "core/io/image_rw.h"
#include "core/io/io.h"
#include "core/io/resource_loader.h"
#include "core/math/math_funcs.h"
#include "core/templates/hash_map.h"
Expand Down Expand Up @@ -475,69 +477,6 @@ int Image::get_mipmap_count() const {
}
}

/// Using template generates perfectly optimized code due to constant expression reduction and unused variable removal present in all compilers.
template <uint32_t read_bytes, bool read_alpha, uint32_t write_bytes, bool write_alpha, bool read_gray, bool write_gray>
static void _convert(int p_width, int p_height, const uint8_t *p_src, uint8_t *p_dst) {
constexpr uint32_t max_bytes = MAX(read_bytes, write_bytes);

for (int y = 0; y < p_height; y++) {
for (int x = 0; x < p_width; x++) {
const uint8_t *rofs = &p_src[((y * p_width) + x) * (read_bytes + (read_alpha ? 1 : 0))];
uint8_t *wofs = &p_dst[((y * p_width) + x) * (write_bytes + (write_alpha ? 1 : 0))];

uint8_t rgba[4] = { 0, 0, 0, 255 };

if constexpr (read_gray) {
rgba[0] = rofs[0];
rgba[1] = rofs[0];
rgba[2] = rofs[0];
} else {
for (uint32_t i = 0; i < max_bytes; i++) {
rgba[i] = (i < read_bytes) ? rofs[i] : 0;
}
}

if constexpr (read_alpha || write_alpha) {
rgba[3] = read_alpha ? rofs[read_bytes] : 255;
}

if constexpr (write_gray) {
// REC.709
const uint8_t luminance = (13938U * rgba[0] + 46869U * rgba[1] + 4729U * rgba[2] + 32768U) >> 16U;
wofs[0] = luminance;
} else {
for (uint32_t i = 0; i < write_bytes; i++) {
wofs[i] = rgba[i];
}
}

if constexpr (write_alpha) {
wofs[write_bytes] = rgba[3];
}
}
}
}

template <typename T, uint32_t read_channels, uint32_t write_channels, T def_zero, T def_one>
static void _convert_fast(int p_width, int p_height, const T *p_src, T *p_dst) {
uint32_t dst_count = 0;
uint32_t src_count = 0;

const int resolution = p_width * p_height;

for (int i = 0; i < resolution; i++) {
memcpy(p_dst + dst_count, p_src + src_count, MIN(read_channels, write_channels) * sizeof(T));

if constexpr (write_channels > read_channels) {
const T def_value[4] = { def_zero, def_zero, def_zero, def_one };
memcpy(p_dst + dst_count + read_channels, &def_value[read_channels], (write_channels - read_channels) * sizeof(T));
}

dst_count += write_channels;
src_count += read_channels;
}
}

static bool _are_formats_compatible(Image::Format p_format0, Image::Format p_format1) {
if (p_format0 <= Image::FORMAT_RGBA8 && p_format1 <= Image::FORMAT_RGBA8) {
return true;
Expand All @@ -551,6 +490,7 @@ static bool _are_formats_compatible(Image::Format p_format0, Image::Format p_for
}

void Image::convert(Format p_new_format) {
IO::Error err = IO::Error::Okay;
ERR_FAIL_INDEX_MSG(p_new_format, FORMAT_MAX, vformat("The Image format specified (%d) is out of range. See Image's Format enum.", p_new_format));

if (data.is_empty() || p_new_format == format) {
Expand Down Expand Up @@ -592,185 +532,81 @@ void Image::convert(Format p_new_format) {
// Convert the formats in an optimized way by removing/adding color channels if necessary.
Image new_img(width, height, mipmaps, p_new_format);

const int conversion_type = format | p_new_format << 8;

for (int mip = 0; mip < mipmap_count; mip++) {
int64_t mip_offset = 0;
int64_t mip_size = 0;
int mip_width = 0;
int mip_height = 0;
IO::Reader reader = {};
IO::Writer writer = {};
IO::Image::Reader imReader = {};
IO::Image::Writer imWriter = {};
ColorRGBAF32x16 block;
get_mipmap_offset_size_and_dimensions(mip, mip_offset, mip_size, mip_width, mip_height);

const uint8_t *rptr = data.ptr() + mip_offset;
uint8_t *wptr = new_img.data.ptrw() + new_img.get_mipmap_offset(mip);

switch (conversion_type) {
case FORMAT_L8 | (FORMAT_LA8 << 8):
_convert<1, false, 1, true, true, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_R8 << 8):
_convert<1, false, 1, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_RG8 << 8):
_convert<1, false, 2, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_RGB8 << 8):
_convert<1, false, 3, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_RGBA8 << 8):
_convert<1, false, 3, true, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_L8 << 8):
_convert<1, true, 1, false, true, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_R8 << 8):
_convert<1, true, 1, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_RG8 << 8):
_convert<1, true, 2, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_RGB8 << 8):
_convert<1, true, 3, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_RGBA8 << 8):
_convert<1, true, 3, true, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_L8 << 8):
_convert<1, false, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_LA8 << 8):
_convert<1, false, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_RG8 << 8):
_convert<1, false, 2, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_RGB8 << 8):
_convert<1, false, 3, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_RGBA8 << 8):
_convert<1, false, 3, true, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_L8 << 8):
_convert<2, false, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_LA8 << 8):
_convert<2, false, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_R8 << 8):
_convert<2, false, 1, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_RGB8 << 8):
_convert<2, false, 3, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_RGBA8 << 8):
_convert<2, false, 3, true, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_L8 << 8):
_convert<3, false, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_LA8 << 8):
_convert<3, false, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_R8 << 8):
_convert<3, false, 1, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_RG8 << 8):
_convert<3, false, 2, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_RGBA8 << 8):
_convert<3, false, 3, true, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_L8 << 8):
_convert<3, true, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_LA8 << 8):
_convert<3, true, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_R8 << 8):
_convert<3, true, 1, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_RG8 << 8):
_convert<3, true, 2, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_RGB8 << 8):
_convert<3, true, 3, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RH | (FORMAT_RGH << 8):
_convert_fast<uint16_t, 1, 2, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RH | (FORMAT_RGBH << 8):
_convert_fast<uint16_t, 1, 3, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RH | (FORMAT_RGBAH << 8):
_convert_fast<uint16_t, 1, 4, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGH | (FORMAT_RH << 8):
_convert_fast<uint16_t, 2, 1, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGH | (FORMAT_RGBH << 8):
_convert_fast<uint16_t, 2, 3, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGH | (FORMAT_RGBAH << 8):
_convert_fast<uint16_t, 2, 4, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBH | (FORMAT_RH << 8):
_convert_fast<uint16_t, 3, 1, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBH | (FORMAT_RGH << 8):
_convert_fast<uint16_t, 3, 2, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBH | (FORMAT_RGBAH << 8):
_convert_fast<uint16_t, 3, 4, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBAH | (FORMAT_RH << 8):
_convert_fast<uint16_t, 4, 1, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBAH | (FORMAT_RGH << 8):
_convert_fast<uint16_t, 4, 2, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBAH | (FORMAT_RGBH << 8):
_convert_fast<uint16_t, 4, 3, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RF | (FORMAT_RGF << 8):
_convert_fast<uint32_t, 1, 2, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RF | (FORMAT_RGBF << 8):
_convert_fast<uint32_t, 1, 3, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RF | (FORMAT_RGBAF << 8):
_convert_fast<uint32_t, 1, 4, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGF | (FORMAT_RF << 8):
_convert_fast<uint32_t, 2, 1, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGF | (FORMAT_RGBF << 8):
_convert_fast<uint32_t, 2, 3, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGF | (FORMAT_RGBAF << 8):
_convert_fast<uint32_t, 2, 4, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBF | (FORMAT_RF << 8):
_convert_fast<uint32_t, 3, 1, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBF | (FORMAT_RGF << 8):
_convert_fast<uint32_t, 3, 2, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBF | (FORMAT_RGBAF << 8):
_convert_fast<uint32_t, 3, 4, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBAF | (FORMAT_RF << 8):
_convert_fast<uint32_t, 4, 1, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBAF | (FORMAT_RGF << 8):
_convert_fast<uint32_t, 4, 2, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBAF | (FORMAT_RGBF << 8):
_convert_fast<uint32_t, 4, 3, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
err = IO::Reader::make(
&reader,
{
.data = (void *)rptr,
.length = (size_t)mip_size,
});
if (err != IO::Error::Okay) {
return;
}
err = IO::Image::Reader::make(
&imReader,
reader,
format,
mip_width,
mip_height);
if (err != IO::Error::Okay) {
IO::Reader::destroy(&reader);
return;
}
Comment on lines +558 to +567

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Unify the failure exits in the per-mip pipeline. Each make failure is handled ad hoc — one breaks, two return — so no path both tears down the already-constructed IO objects and prevents the destination image from being published. Introduce a single labeled cleanup/abort path (or RAII wrappers) that destroys whatever was constructed, reports the failure, and returns without touching new_img.

  • core/io/image.cpp#L555-L563: replace the break with the abort path so a reader-construction failure cannot fall through to _copy_internals_from(new_img) at Line 599, and destroy reader before returning.
  • core/io/image.cpp#L565-L582: replace both bare returns with the same abort path so reader/imReader (Line 572) and reader/imReader/writer (Line 581) are destroyed, and surface the error via ERR_FAIL_MSG instead of returning silently.
📍 Affects 1 file
  • core/io/image.cpp#L555-L563 (this comment)
  • core/io/image.cpp#L565-L582
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/io/image.cpp` around lines 555 - 563, Unify the per-mip failure handling
around the IO::Image::Reader::make and related construction calls in
core/io/image.cpp:555-563 and core/io/image.cpp:565-582 by replacing the break
and bare returns with one cleanup/abort path. Ensure it destroys every object
constructed up to each failure, reports the error through ERR_FAIL_MSG, returns
before _copy_internals_from(new_img), and never publishes new_img; both
consolidated sites require these changes.

new_img.get_mipmap_offset_and_size(mip, mip_offset, mip_size);
err = IO::Writer::make(
&writer,
{
.data = (void *)wptr,
.length = (size_t)(mip_size),
});
if (err != IO::Error::Okay) {
IO::Image::Reader::destroy(&imReader);
IO::Reader::destroy(&reader);
return;
}
err = IO::Image::Writer::make(
&imWriter,
writer,
new_img.format,
mip_width,
mip_height);
if (err != IO::Error::Okay) {
IO::Image::Reader::destroy(&imReader);
IO::Reader::destroy(&reader);
IO::Writer::destroy(&writer);
return;
}
Comment on lines +569 to +591

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Early returns skip IO teardown.

Returning at Line 572 leaves reader/imReader constructed, and Line 581 additionally leaves writer constructed; the destroy hooks at Lines 590-593 are bypassed, so any state allocated by make leaks. Route all failure exits through one cleanup path (and report the failure, e.g. ERR_FAIL_MSG, rather than returning silently).

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/io/image.cpp` around lines 565 - 582, Update the image processing flow
containing IO::Writer::make and IO::Image::Writer::make so every construction
failure routes through a shared cleanup path instead of returning immediately.
Ensure that path destroys reader, imReader, writer, and imWriter as appropriate,
reports the IO error with ERR_FAIL_MSG, and preserves normal teardown on
success.

do {
err = IO::Image::Reader::read(imReader, &block);
if ((err == IO::Error::Okay) | (err == IO::Error::Eof)) {
err = IO::Image::Writer::write(imWriter, &block);
}
} while (err == IO::Error::Okay);
err = IO::Image::Writer::flush(imWriter);
IO::Image::Reader::destroy(&imReader);
IO::Image::Writer::destroy(&imWriter);
IO::Reader::destroy(&reader);
IO::Writer::destroy(&writer);
if (err != IO::Error::Okay) {
return;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

_copy_internals_from(new_img);
return;
}

Image::Format Image::get_format() const {
Expand Down
12 changes: 12 additions & 0 deletions core/io/image.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,18 @@ typedef Error (*SaveDDSFunc)(const String &p_path, const Ref<Image> &p_img);
typedef Vector<uint8_t> (*SaveDDSBufferFunc)(const Ref<Image> &p_img);
/// @}

// by 16 for avx512 eventually (TM)
union alignas(64) ColorRGBAF32x16 {
struct
{
float r[16];
float g[16];
float b[16];
float a[16];
};
float c[4][16];
};

class Image : public Resource {
GDCLASS(Image, Resource);

Expand Down
Loading
Loading