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

Add ASTC Normalmap support #23

Closed
wants to merge 2 commits into from

Conversation

@cloudwu
Copy link
Contributor

commented Jul 9, 2019

See bkaradzic/bgfx#1788

This PR include two patchs, the first one updats orignal astc-encoder to the latest.
The second one d5af27e adds normal map support. See https://github.com/ARM-software/astc-encoder/blob/master/Docs/Encoding.md#encoding-normal-maps

It just merged code from https://github.com/ARM-software/astc-encoder/blob/master/Source/astc_toplevel.cpp#L1588-L1642

I add some enums in astc_lib.h , ASTC_COMPRESS_MODE::ASTC_COMPRESS_NORMAL_PSNR , ASTC_COMPRESS_MODE::ASTC_COMPRESS_NORMAL_PERCEP, and ASTC_CHANNELS::ASTC_RA .

We can compress a normal map texture by

// Use ASTC_COMPRESS_NORMAL_PERCEP for better quailty.
astc_compress(_width, _height, src, ASTC_RA, srcPitch, astcBlockInfo.blockWidth, 
      astcBlockInfo.blockHeight, ASTC_COMPRESS_NORMAL_PSNR, decode_mode, dst);

https://github.com/bkaradzic/bimg/blob/master/src/image_encode.cpp#L146

@andrewwillmott Could you review this PR ? I hope nothing is wrong.

I haven't change the behaviour of bimg itself , I guess we need to add an new api of encoder , so that the encoder can encode the texture in normal map mode. bimg has already have an command line option --normal , @bkaradzic Can you add an new API like bimg::imageEncodeNormalMapFromRgba32f ? Or you may have a better idea.

@cloudwu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@bkaradzic Could you merge this one first? and I can do the rest part.

@andrewwillmott

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Hi, sorry for the radio silence, I've been slammed and I forgot about the email notification.

The astc lib changes look ok to me on a once-over. I've synced them and I'll test them later today.

The library-ised version of the ARM code bimg uses lives here: https://github.com/andrewwillmott/astc-encoder. So it's be good to get these changes there too. I'll just check they haven't diverged already first.

@cloudwu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@andrewwillmott Do you need me to create the same PR (Adding ASTC_RA) to https://github.com/andrewwillmott/astc-encoder ? or you can merge it by yourself.

@bkaradzic

This comment has been minimized.

Copy link
Owner

commented Jul 15, 2019

@cloudwu Yeah, it's better to create first PR for astc-encoder, and merge it there, and then copy changes here (workflow should be the same as if bimg used submodules).

@andrewwillmott

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Yeah, tell you what -- if you could create a PR against astc-encoder, I'll merge it and test it and then handle the updating of the bgfx copy.

@andrewwillmott

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Actually, never mind, you can leave it to me, the normal map patch applies to the original without the update at least.

@andrewwillmott

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

I've added your changes to https://github.com/andrewwillmott/astc-encoder and then slightly refactored them, to allow RA->XYZ decoding, and the encode-for-normal to be independent of the quality. (Though of course they set some of the same things.)

I'm going to hold off on updating from https://github.com/ARM-software/astc-encoder for now. Some of the recent changes are about ripping out 32-bit support and requiring C++11, and I suspect some bgfx/astc_lib users are not ready to accept that yet. Also there're a lot of changes that have gone in in the last month, after it has been fairly inactive for a long time prior to that, so it might be an idea to let them settle. The only bugfix I can find is to astc_toplevel which isn't used in the library variant, so I don't think we're missing anything.

I'll bring these changes across to bimg and in doing so have a look at how they should be called from image_encode.cpp. I'm guessing by adding more TextureFormats, though the number of block formats in ASTC makes this annoying.

@cloudwu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

I'll close this PR after @bkaradzic copy the changes from https://github.com/andrewwillmott/astc-encoder , or @andrewwillmott may create a new PR ?

@andrewwillmott

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

I'll create a new PR, yeah, with the texturec changes.

Did you find one of the normal map encoding options was better than the other? It'd be easier to expose just the one in the tool due to the API.

@cloudwu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

I'll create a new PR, yeah, with the texturec changes.

Did you find one of the normal map encoding options was better than the other? It'd be easier to expose just the one in the tool due to the API.

I am porting https://assetstore.unity.com/packages/essentials/tutorial-projects/viking-village-29140 this scene from unity to bgfx these days. I'll compare two encoding options next week. I think one option is ok , we can choose the better one by default.

@cloudwu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Some comparison for your information :

I use a normal map from https://github.com/fmod/Viking-Village/blob/master/Assets/Textures/Terrain/terrain_01_n.tif (A 2048x2048 tangent-space normal map)

encode tools format channels bpp command options PSNR (dB) higher is better
nvcompress dxt1nm RGB 4 -nomips -normal -bc1n 27.484683
astcenc astc 6x5 RG 4.27 -t 4.0 -normal_psnr -thorough 29.670458
astcenc astc 4x4 RGB 8 -t 8.0 -thorough 33.238881
astcenc astc 5x4 RG 6.4 -t 6.0 -normal_psnr -thorough 33.839983
astcenc astc 5x4 RG 6.4 -t 6.0 -normal_psnr -thorough 33.839983
nvcompress dxt5nm RG 8 -nomips -normal -bc3n 34.518504
astcenc astc 4x4 RG 8 -t 8.0 -normal_percep -fast 35.779205
astcenc astc 4x4 RG 8 -t 8.0 -normal_psnr -fast 36.121064
astcenc astc 4x4 RG 8 -t 8.0 -normal_percep -thorough 37.030079
astcenc astc 4x4 RG 8 -t 8.0 -normal_psnr -thorough 37.403131
etcpack EAC_RG11 RG 8 -f RG 40.834371
  • I strip the blue channel for all the results and then use astcenc -compare to calcuate (only two channels) PSNR.
  • EAC_RG11 has the best PSNR , but etcpack is very very slow (It cost a few minutes to encode this texture) .
  • I think we can use -normal_percep for astcenc by default, becuase etcpack has a similar option by default.

btw, @bkaradzic Could you add an option to use stereographic projection for two-channels normal map ? See https://www.nvidia.com/object/real-time-normal-map-dxt-compression.html (3. Tangent-Space Normal Maps)

@cloudwu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

btw, etcpack use a very slow algorithm to encode EAC format, my implemention is much faster (reduce from O(n*n) to O(n) ) . bkaradzic/bgfx#1392

@andrewwillmott

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

I have this working in bimg/texturec, but I'm finding the astc code is giving me compression artefacts on this test image.

sphere-nmap

Still digging into that. One thing to note is the astc -normal options overlap with -fast/-thorough etc., this is why the difference between the fast/thorough options is small. The normal map modes tend to force very thorough encoding modes, maybe to try to deal with the artefacts?

From looking at the table, I've added just ASTC_4x4N and ASTC_6x6N as the normal map encoding options.

@andrewwillmott

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

I traced the block artefacts to zero encode weights for gb, epsilon values fixed them. I think this may have been the reason for the high encode thresholds the -normal mode sets in astcenc, which override some of the compression mode settings. Hence I'm now leaving the those to the compression speed setting, so the two things are independent.

I've just opened #24 which brings bimg up to date with my astc_encoder fork and makes it so the -n/-normalmap option triggers the ASTC_ENC_NORMAL_RA mode. Unfortunately I initially went down the path of doing this by adding corresponding bimg TextureFormat enums, before realising that would require a fair few changes to bgfx, due to the enum shadowing, and switching to using Quality instead.

@cloudwu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

I think we may change the prototype of imageEncodeFromRgba32f by adding an additional options bit flags to support more encoding algorithm.

btw, @bkaradzic bgfx haven't support BC3nm/DXT5nm now, could you add it ? At least we can support encoding BC3nm in bimg as BC3 by channel swizzling .

@cloudwu cloudwu closed this Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.