Skip to content

Conversation

@Zylann
Copy link
Contributor

@Zylann Zylann commented Jun 10, 2018

This adds support for Image.FORMAT_R16, which stores one channel as 16-bit integers.
They can be saved or loaded as 16-bit gray PNG (based a little on #19391).
They can also be rendered as textures using the GL_R16 internal format.

The use case for such textures are high-precision depth maps, or height maps for making terrains with a better precision than the RH format (half-precision float) while keeping the same memory footprint. It can also allow to directly use maps produced by other software such as WorldMachine, or Gimp.

Notes:

  • I initially wanted the format to be L16 with GL_LUMINANCE16. While textures were correctly uploading, it was rendering as black and I couldn't find why. Using GL_R16 worked so that's why I used it.
  • Previews show up as black and white, I'm not sure why. They will normally render as red if used as is on a sprite.
  • It looks like GL_R16 isn't recognized by compilers for Android and iOS :( are they misconfigured or is it really not supported at all there? Or maybe another format has to be picked? GL_R16UI appears to be in common but does it mean shaders will need to sample that as an ushort and normalize themselves?

@Zylann Zylann requested review from karroffel and reduz as code owners June 10, 2018 19:33
@reduz
Copy link
Member

reduz commented Jun 13, 2018

The problem with this PR, which is why I think it should not be merged (or not as is at least) is that I remember finding software that by default saves PNGs to 16 bits without the user knowing.

This will result in twice the amount of memory used and no backwards compatibility (will not work on GLES2) for people making 2D games and will have no clue what is going on.

Why not using floats for half floats for what you are doing? It does not seem like it should change that much.

@akien-mga
Copy link
Member

GL_R16 is not available in the OpenGL ES 3.0 specification (it's in 3.2 though, but we don't use that). GL_R16UI might work, it's in ES 3.0: https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glTexImage2D.xhtml
I don't know about ES 2.0 though (OpenGL ES docs are terrible to find a proper reference of what internal sized formats are supported...).

@reduz
Copy link
Member

reduz commented Jun 13, 2018

If I were to do something like what you are doing, I would just use floats and then I would save to a custom binary format that reads min/max of the section and then converts to a normalized 16 bits PNG on save, using a custom, binary format.

@Zylann
Copy link
Contributor Author

Zylann commented Jun 13, 2018

I understand the PNG concern, so I'm ok if that part isn't merged.

However, I thought it was obvious already, but I'm saying again:
I wanted to be able to use 16-bit integer because half float is loosing precision really fast when you do terrains with high altitude, and it also shows when calculating normals out of them. Using 32-bit floats is quite a drastic decision because it doubles your memory footprint going out of your budget, which is pretty bad on textures as big as terrains. With 16-bit integer you can use the whole allowed range for the height you need, with the same precision at any altitude, giving better results than half floats.
All terrain systems I've studied so far use this format... it's too bad it needs to be scrapped due to mobiles and old drivers :/
Also, can't we just use GL_R16UI ? My PR doesn't compile just because I need to know if we can use it (genuine question, I noted this format exists is in both specs, contrary to GL_R16 (also Unity supports it))

@mhilbrunner
Copy link
Member

Note there are multiple people requesting 16bit PNG suppot (#19391) not sure I'd want to drop that.

@logzero
Copy link

logzero commented Jul 30, 2018

Yeah, it would be nice to at least have R16UI as an option, as half float can only correctly represent integers in [-2048, 2048] range.

The default can be some noob friendly precision lowering conversion, if it's really an issue.

@akien-mga
Copy link
Member

Moving to 3.2 milestone as we're about to enter the beta stage and release freeze for Godot 3.1. Only bug fixes against issues of the 3.1 milestone (or enhancements to core features of the 3.1 roadmap) will be considered until 3.1-stable is released.

Note: If it's merged after 3.1-stable is released, this change might be considered for cherry-picking into a later 3.1.x release.

@aaronfranke
Copy link
Member

What's the status of 16-bit support in Vulkan? This may be good to put on the 4.0 milestone.

@akien-mga akien-mga removed the request for review from karroffel August 28, 2019 10:26
@akien-mga
Copy link
Member

We discussed this on IRC today with @reduz. He's still unconvinced due to the potential impact of tools wrongly saving PNGs as 16-bit. He agreed that it could be added nevertheless if it's made opt-in via an option in the PNG format loader (and the saver might not need it, as it would just infer it from the Image format).

This would have to wait for 4.0, so either a new PR should be made against the vulkan branch now, or this one could be rebased on top of the master branch once vulkan is merged after the 3.2 release.

Logs:

14:34 <Akien> Next: #19497
14:34 <IssueBot> #19497: Added support for 16-bit gray PNG, Image and Texture | https://git.io/vhasw
14:35 <Akien> I guess it doesn't make much sense to merge now for 3.2, but will it be considered in this form for 4.0, or should something different be implemented?
14:35 <reduz> I already expressed my view on this one, I would close it
14:36 <reduz> the problem is that one thing is 16 bits texures and the other is floating point textures
14:36 <reduz> 16 bits textures dont make much sense
14:37 <reduz> and supporting this will probably break a huge amount of existing projects because some tools save PNGs as 16 bits without the user knowing, like imagemagick or photoshop
14:37 <reduz> and asking users to pay attention to this is overkill
14:38 <reduz> i dont deny it can be useful, but the confusion potential is much greater than the usability potential
14:41 <Akien> Can we add support for it without changing the PNG importer to auto-use it?
14:41 <Akien> So users who know what they're doing can manually import PNGs as 16-bit
14:41 <reduz> no, i dont think so, not without a lot of hack
14:42 <reduz> we have proper floating point textures such as EXR
14:42 <reduz> but they are not as compressible
14:43 <Akien> Can't it be a PNG import option, like import_as_16_bit = false?
14:43 <Akien> There are quite a few users who support this PR, so it seems there's a valid use case.
14:44 <reduz> it could be an option in the image loader i guess
14:44 <reduz> which we also pass to the importer
14:44 <reduz> it needs to be passed everywhere
14:44 <Akien> Right that's what I meant, the ResourceFormatLoaderPNG
14:44 <reduz> i guess its enough for the loader, the saver can detect it from the format
14:45 <reduz> or maybe not because Image actually does not support 16 bits
14:45 <reduz> we would need to add support for 16 bits support in Image too
14:45 <Akien> Well this PR adds support for it in Image
14:45 <reduz> oh ok
14:45 <Akien> https://github.com/godotengine/godot/pull/19497/files
14:45 <reduz> i forgot about that
14:45 <reduz> so i guess then, we should most likely add this for 4.0 then
14:47 <reduz> we can discuss it with Zylann and ask him to do a PR for the vulkan branch
14:47 <Akien> Sounds good. I'll paste the logs on the PR and tell him to either redo the PR for the vulkan branch, or wait for vulkan to be merged and rebase that PR on master then.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Aug 28, 2019
@aaronfranke aaronfranke marked this pull request as draft April 9, 2020 01:34
@aaronfranke
Copy link
Member

@Zylann Is this still desired? If so, it needs to be rebased on the latest master branch, and the above feedback needs to be addressed.

If not, abandoned pull requests will be closed in the future as announced here.

@Zylann
Copy link
Contributor Author

Zylann commented May 13, 2020

@aaronfranke it was, but now EXR support got improved so personally I no longer need this as much. I'm more looking for that in EXR Zylann/godot_heightmap_plugin#34
So unless other people still need this resource format I guess it can be archived for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants