Open Bioblaze opened 2 months ago
Made a realization that I don't know where the texture is... within the atlas... fun fun fun.
in response to your discord question (yes i know, im a bit late, not sure if this is still relevant):
ordered roughly as encountered, things already mentioned are not repeated
comments: in general these should explain why and not what. what the code is doing should be obvious from the names of variables & functions. for functions and classes doc comments can provide additional detail about their behavior & requirements that cannot be expressed by their signature.
stringly typed paths: why not use filesystem::path everywhere? it clearly expresses what it represents through the type-system and comes with build-in path related functionality. (the value_type of path is OS dependent however)
isPng: its technically only checking the extension, a bit pedantic, but hasPngExtension would say what it is actually doing. the check on the extension does create 3 paths, with optimization this might not be an issues, and for this usage it does not matter as it is not performance critical, just wanted to mention it.
createTextureAtlas: !exists(file) & !isPng(file) are handled differently, which means that client code has to use multiple error handling strategies to check for success. just use one. Magick::Image::read looks like it can fail as well but that is not handled idx could move inside the for. why is a copy of outputFilename returned? the caller already has access to it, no need to return a copy, returning a bool would be enough.
atlasToHeader: can there only ever be 1 atlas header? if not then the header guard would need the be different per header. headerFile.close(); is redundant.
in response to your discord question (yes i know, im a bit late, not sure if this is still relevant):
ordered roughly as encountered, things already mentioned are not repeated
comments: in general these should explain why and not what. what the code is doing should be obvious from the names of variables & functions. for functions and classes doc comments can provide additional detail about their behavior & requirements that cannot be expressed by their signature.
stringly typed paths: why not use filesystem::path everywhere? it clearly expresses what it represents through the type-system and comes with build-in path related functionality. _(the valuetype of path is OS dependent however)
isPng: its technically only checking the extension, a bit pedantic, but hasPngExtension would say what it is actually doing. the check on the extension does create 3 paths, with optimization this might not be an issues, and for this usage it does not matter as it is not performance critical, just wanted to mention it.
createTextureAtlas: !exists(file) & !isPng(file) are handled differently, which means that client code has to use multiple error handling strategies to check for success. just use one. Magick::Image::read looks like it can fail as well but that is not handled idx could move inside the for. why is a copy of outputFilename returned? the caller already has access to it, no need to return a copy, returning a bool would be enough.
atlasToHeader: can there only ever be 1 atlas header? if not then the header guard would need the be different per header. headerFile.close(); is redundant.
Thank you very much! I'll push v3 of the script later today, and i'll make sure I also take into account your suggestions ^_^
Generating a texture Atlas for a series of Textures.