darksylinc / betsy

Betsy GPU compressor
Other
322 stars 18 forks source link

Some proposals #7

Open feliwir opened 3 years ago

feliwir commented 3 years ago

This is more of a meta PR, asking for changes that make sense to you / you'd accept a PR for:

darksylinc commented 3 years ago

OK this will take some time to review, a few comments:

  1. I'm a bit hesitant about FreeImage removal. stb_image was explicitly rejected because it was written for video game engines in mind (meaning: if the file doesn't open with stb_image then you save it again with different setting or extension). std_image won't handle all variations (e.g. like interlaced JPG). On the other hand, it's leaner and worth evaluating if the user shouldn't just re-export the texture instead of expecting betsy to accept anything you throw at it
  2. Your PR overwrites .clang-format, the final PR must not.
  3. GLFW: If it's readable, leaner and works then great!
  4. GLI abstraction: Sounds cool!!! at least in theory.
    • Ok I took a look at the commit and right now it's only mostly swapping our enums with GLI's, therefore I don't understand yet what GLI brings to the table.

Betsy was commissioned for Godot, thus the way the code was structured was to maximize easiness of integration into other projects, which is why the classes have abstracted GL code away. A key aspect we wish to conserve is that integration (aka copy paste into your own project) should be straightforward. As long as this aspect is preserved I'm cool with it. I wanted to go with Vulkan but time was limited and Vulkan initial setup is harder and we were on a tight budget; which is why we went for OpenGL (they both support GLSL anyway, and 95% of the project is GLSL code)

feliwir commented 3 years ago

So to answer some questions:

  1. I didn't like the way we pulled some sources from FreeImage into this repository. We should either use a real FreeImage package with find_package or do something else.
  2. Yea, that was for my fork. I'd remove that and make a separate PR for each feature.
  3. good
  4. GLI has the advantage that it can also translate to DirectX and Vulkan formats. As you probably know best yourself the KTX format does store the format parameters in OpenGL internal format specifiers. But e.g. DDS does use the internal DirectX format. So e.g. if we wanted to support DDS (which i definetly want to add aswell, since it's still the most common for compressed textures we need to be able to translate to that aswell. Another benefit of GLI is the mentioned Vulkan support, which it can translate to aswell. It looks like that:
    gli::gl GL(gli::gl::PROFILE_GL33);
    gli::gl::format const GLFormat = GL.translate(Texture.format(), Texture.swizzles());
        gli::dx DX();
        gli::dx::format const DXFormat = DX.translate(Texture.format(), Texture.swizzles());

    (and so on for Vulkan)

  5. Another point i forgot: I compiled the shaders into the binary and did the preprocessing with cmake. So this should reduce the headache of requiring the shaders next to the binary and also reduce startup time, since no shader preprocessing & file io is performed.
darksylinc commented 3 years ago

GLI has the advantage that it can also translate to DirectX and Vulkan formats. As you probably know best yourself the KTX format does store the format parameters in OpenGL internal format specifiers. But e.g. DDS does use the internal DirectX format. So e.g. if we wanted to support DDS (which i definetly want to add aswell, since it's still the most common for compressed textures we need to be able to translate to that aswell. Another benefit of GLI is the mentioned Vulkan support, which it can translate to aswell. It looks like that:

Oh I see where it's going. More context:

If we're going to use GLI to support Pre-DX10 headers then it makes sense (assuming GLI supports pre-DX10 dds). Otherwise, integrating a large library like GLI just for format conversion sounds overkill when it's only a large switch statement per backend.

  1. Another point i forgot: I compiled the shaders into the binary and did the preprocessing with cmake. So this should reduce the headache of requiring the shaders next to the binary and also reduce startup time, since no shader preprocessing & file io is performed.

OK that sounds very useful! As long as it can be toggled on/off at build time (both for development and for platforms where embedding the files into the exe can prove difficult)