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.

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

get HDR working #407

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

Conversation

lenscas
Copy link
Contributor

@lenscas lenscas commented Aug 26, 2023

This PR should get HDR working. However, as HDR requires openGL3 I added a feature to enable that.

If this feature is enabled then any fall backs to opengl2 are turned into panics instead.

0,
0,
];
let attribs: [libc::c_int; 8] = if(cfg!(feature="opengl3")){
Copy link
Owner

Choose a reason for hiding this comment

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

opengl will create the highest possible context version despite those number, so its totally fine to leave 2.1 here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A system that only has 2.1 won't be able to use textures with the RGBA16 format. So this way opengl3 will always be loaded, no? Or will it also load lower versions and thus a check needs to happen to make sure that opengl3 got loaded?

Copy link
Owner

Choose a reason for hiding this comment

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

The idea here - if the system only has 2.1 - let it create it's 2.1 context, but turn on all the compatibility code to make the app somewhat working on 2.1.

If we can't do software emulation or other techniques - let the user to decide if the app should print a nice panic message or do some workarounds for missing features on runtime.

Cargo.toml Outdated
@@ -18,6 +18,7 @@ categories = ["rendering::graphics-api"]
# Optional log-rs like macros implementation
# disabled by default
log-impl = []
opengl3 = []
Copy link
Owner

Choose a reason for hiding this comment

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

this is not particullary the way miniquad handle different GL versions.
you can check the instanced for a reference of a feature that is not available on certain GL versions but is still used in miniquad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is normally done through a fallback. however opengl2 doesn't support the texture format that is needed for HDR to work. This way I can remove that variant from the TextureFormat if there is a chance that openGL2 gets loaded instead of 3.

I am happy to change how it works but to me this sounded the most safe.

@not-fl3
Copy link
Owner

not-fl3 commented Aug 26, 2023

Thanks for PR!

Generally I am totally fine with miniquad having platform-specific features, nothing wrong with that, we already have one - instancing.

It works slightly different tho, the prefered way to deal with platform-specific features is:

  • after creation of the context check what do we actually can do and fill the capabilities struct
  • later when the feature is requested it should complain(or, ideally, do some software emulation) for the missing feature
  • there should be a way for a user to check if the feature is present

@lenscas
Copy link
Contributor Author

lenscas commented Aug 26, 2023

I am perfectly fine changing it to s runtime check, however because it touches something as important as textures I figured that having to opt in at compile time would be preferred.

Also, I don't know what happens if you try and do this with opengl2. If it is ub then that means that technically this change causes working with textures to be unsafe or to always need such a check. Either of which doesn't sound ideal to me.

@not-fl3
Copy link
Owner

not-fl3 commented Sep 2, 2023

Generally, platform dependend texture formats would involve a lot of design work. Miniquad should be better on expressing capabilities, but its not yet there.

We tried to do in the past, but it did not worked out really well - https://github.com/not-fl3/miniquad/pull/334/files, the code complexity was not worth the new formats.

@lenscas
Copy link
Contributor Author

lenscas commented Sep 2, 2023

hmm...
What about an API like

pub struct PrivateConstruct(());
pub enum TextureFormat {
    RGB8,
    RGBA8,
    RGBA16(PrivateConstruct),
    Depth,
    Alpha,
}
impl TextureFormat {
  fn rgba16(...) -> Result<Self, Error> {...}
  unsafe fn rgba16_unchecked(...) -> Self {...} // optional
}

It would keep most of the API intact while still forcing people to check only when they want to use RGBA16 and doesn't spread unsafe everywhere.

@not-fl3
Copy link
Owner

not-fl3 commented Sep 2, 2023

hmm... What about an API like

pub struct PrivateConstruct(());
pub enum TextureFormat {
    RGB8,
    RGBA8,
    RGBA16(PrivateConstruct),
    Depth,
    Alpha,
}
impl TextureFormat {
  fn rgba16(...) -> Result<Self, Error> {...}
  unsafe fn rgba16_unchecked(...) -> Self {...} // optional
}

It would keep most of the API intact while still forcing people to check only when they want to use RGBA16 and doesn't spread unsafe everywhere.

I don't think this very API is implementable(I am not sure what to put in place of those ...), but if it works - sure, update the PR!

@lenscas
Copy link
Contributor Author

lenscas commented Sep 2, 2023

I don't think this very API is implementable (I am not sure what to put in place of those ...)

Unless I am missing something, presumably it is a version check of the openGL version loaded. Worst case, only the unsafe version of that function is possible and it is then up to the user to find some way of restricting the hardware on what it runs.

src/graphics.rs Outdated
TextureFormat::Depth => 2 * square,
TextureFormat::Alpha => 1 * square,
}
}
/// SAFETY:
/// Using this method on openGL versions that does not support RGBA16 textures can cause
Copy link
Owner

Choose a reason for hiding this comment

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

Could you elaborate on the UB it might cause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't know enough about openGL to answer that. All I know about it being UB to create textures with that format on OpenGL2 being UB is because of a comment on the Rust gamedev discord ( https://discord.com/channels/676678179678715904/677286494033018924/1145027947254792263 ).

But reading back I can see how I didn't word that nicely, as it isn't the method itself that can cause this, but then creating a texture with this format that is problematic on version 2 instead of 3. I'll reword it tomorrow to better reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the comment to better reflect what is going on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants