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.

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

Jump to bottom

Clean up Image #98084

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Clean up Image #98084

wants to merge 1 commit into from
+235 −236

Conversation

BlueCube3310
Copy link
Contributor

This PR attempts to clean up the Image class by doing the following:

  • Removing unnecessary comments,
  • Grouping together similar parts of the code, which used to be scattered around the files,
  • Moving some never used inline functions from public to private,
  • Moving function definitions to the .cpp file.

@BlueCube3310 BlueCube3310 requested a review from a team as a code owner October 11, 2024 09:56
@AThousandShips AThousandShips added this to the 4.x milestone Oct 11, 2024
AThousandShips
AThousandShips approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Looks good otherwise, I thin this is a worthwhile change even though noisy

core/io/image.cpp Outdated
@@ -85,17 +85,60 @@ const char *Image::format_names[Image::FORMAT_MAX] = {
"ASTC_8x8_HDR",
};

/* External saver function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External saver function pointers. */
// External saver function pointers.

Prefer single line comments

core/io/image.cpp Outdated
SaveWebPBufferFunc Image::save_webp_buffer_func = nullptr;

/* External loader function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External loader function pointers. */
// External loader function pointers.

core/io/image.cpp Outdated
ScalableImageMemLoadFunc Image::_svg_scalable_mem_loader_func = nullptr;
ImageMemLoadFunc Image::_ktx_mem_loader_func = nullptr;

/* External VRAM compression function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External VRAM compression function pointers. */
// External VRAM compression function pointers.

core/io/image.cpp Outdated
Error (*Image::_image_compress_bptc_rd_func)(Image *, Image::UsedChannels) = nullptr;
Error (*Image::_image_compress_bc_rd_func)(Image *, Image::UsedChannels) = nullptr;

/* External VRAM decompression function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External VRAM decompression function pointers. */
// External VRAM decompression function pointers.

core/io/image.cpp Outdated
void (*Image::_image_decompress_etc2)(Image *) = nullptr;
void (*Image::_image_decompress_astc)(Image *) = nullptr;

/* External packer function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External packer function pointers. */
// External packer function pointers.

core/io/image.h Outdated
@@ -153,6 +203,8 @@ class Image : public Resource {
static ScalableImageMemLoadFunc _svg_scalable_mem_loader_func;
static ImageMemLoadFunc _ktx_mem_loader_func;

/* External VRAM compression function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External VRAM compression function pointers. */
// External VRAM compression function pointers.

core/io/image.h Outdated
@@ -162,24 +214,26 @@ class Image : public Resource {
static Error (*_image_compress_bptc_rd_func)(Image *, UsedChannels p_channels);
static Error (*_image_compress_bc_rd_func)(Image *, UsedChannels p_channels);

/* External VRAM decompression function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External VRAM decompression function pointers. */
// External VRAM decompression function pointers.

core/io/image.h Outdated
static void (*_image_decompress_bc)(Image *);
static void (*_image_decompress_bptc)(Image *);
static void (*_image_decompress_etc1)(Image *);
static void (*_image_decompress_etc2)(Image *);
static void (*_image_decompress_astc)(Image *);

/* External packer function pointers. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* External packer function pointers. */
// External packer function pointers.

core/io/image.h Outdated
Image(int p_width, int p_height, bool p_use_mipmaps, Format p_format); // Create an empty image of a specific size and format.
Image(int p_width, int p_height, bool p_mipmaps, Format p_format, const Vector<uint8_t> &p_data); // Import an image of a specific size and format from a byte vector.
Image(const uint8_t *p_mem_png_jpg, int p_len = -1); // Import either a png or jpg from a pointer.
Image(const char **p_xpm); // Import a XPM image.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Image(const char **p_xpm); // Import a XPM image.
Image(const char **p_xpm); // Import an XPM image.

core/io/image.h Outdated
void _copy_internals_from(const Image &p_image);

_FORCE_INLINE_ Color _get_color_at_ofs(const uint8_t *ptr, uint32_t ofs) const;
_FORCE_INLINE_ void _set_color_at_ofs(uint8_t *ptr, uint32_t ofs, const Color &p_color);

_FORCE_INLINE_ void _get_mipmap_offset_and_size(int p_mipmap, int64_t &r_offset, int &r_width, int &r_height) const; //get where the mipmap begins in data
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_FORCE_INLINE_ void _get_mipmap_offset_and_size(int p_mipmap, int64_t &r_offset, int &r_width, int &r_height) const; //get where the mipmap begins in data
_FORCE_INLINE_ void _get_mipmap_offset_and_size(int p_mipmap, int64_t &r_offset, int &r_width, int &r_height) const; // Get where the mipmap begins in data.

Missed one

@BlueCube3310 BlueCube3310 mentioned this pull request Oct 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips approved these changes

Assignees
No one assigned
Labels
enhancement topic:codestyle topic:core
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.
2 participants
@BlueCube3310 @AThousandShips