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

Artifact in converted .hdr #15

Closed
elalish opened this issue Nov 27, 2023 · 8 comments
Closed

Artifact in converted .hdr #15

elalish opened this issue Nov 27, 2023 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@elalish
Copy link

elalish commented Nov 27, 2023

When I converted this HDR file to JPG using your tool, I found it created an artifact at the equirect seam:
image
As opposed to the correct version with the original .hdr:
image

@daniele-pelagatti
Copy link
Contributor

Interesting, I cannot reproduce this problem if I convert the HDR you posted and then apply it to the example I submitted to three.js.

I'm trying to determine if the bug is coming from the converter (encoding phase) or the library (decoding phase)

This is my converted JPG file (zipped because you never now if online services like github will strip the gain map metadata or not).
ARHusky_09.zip, can you try it in your scene and tell me if it still causes the problem?

If it doesn't cause problems then the problem must be in the encoding phase: can you post your converted JPG and maybe tell me the hardware you used to do the conversion?
If it still causes problems then the problem must be on your usage of the decoder (mipmaps maybe?).

Let's see

@elalish
Copy link
Author

elalish commented Nov 28, 2023

Thanks, I do see the same problem with your JPG. I tried disabling mipmap generation like your example, but it still repros. Also, why do you disable mipmap generation? That should work fine on a half-float texture, right?

Also, your JPG does repro the issue for me in the three.js example (I'm on a Macbook Pro, OS 14.1.1, using Chrome):
image

@daniele-pelagatti
Copy link
Contributor

ok good to know it's not a fault in the encoding process (more complicated :D ) , tomorrow I'll try to reproduce the problem on your hardware and, if it's mipMap related then #17 should solve it.

@daniele-pelagatti
Copy link
Contributor

Also, why do you disable mipmap generation? That should work fine on a half-float texture, right?

I do not disable it, rather, I suspect it is enabled where it shouldn't be and this seems to cause problems, #17 allows for complete control over when to enable or disable it and disables it by default, we'll see if this is the case, I'll let you know

@elalish
Copy link
Author

elalish commented Nov 28, 2023

The plot thickens - this does not repro for me on Safari. My best guess is some kind of undefined behavior in the shader?

@elalish
Copy link
Author

elalish commented Nov 28, 2023

Also strange, on Chrome I see the error in the background skybox, but not in the PMREM reflection, so I guess this has something to do with the conversion to a cubemap?

@daniele-pelagatti
Copy link
Contributor

Alright!

The problem did not manifest when using toDataTexture() in order to generate the background image but I was able to reproduce the problem on your hardware + the dev version of threejs which allows to skip the toDataTexture() "hack" in order to use the equirectangular rendertarget as scene background.

I can confirm #17 solves it. I'm not sure exactly why this was happening but the conversion of the renderTarget.texture to cubemap suffers when the renderTarget is created with generateMipmaps = true, this can be seen at the poles with any texture, including spruit sunrise

this is a comparison of v2.07 vs the upcoming v3.0.0 with #17 applied

before

hdrJpg = new HDRJPGLoader( renderer )
	.load( 'textures/gainmap/ARHusky_09.jpg', function ( ) {

		hdrJpgPMREMRenderTarget = pmremGenerator.fromEquirectangular( hdrJpg.renderTarget.texture );
		hdrJpgEquirectangularMap = hdrJpg.renderTarget.texture;
		hdrJpgEquirectangularMap.mapping = THREE.EquirectangularReflectionMapping;
		hdrJpgEquirectangularMap.minFilter = THREE.LinearFilter;
		hdrJpgEquirectangularMap.magFilter = THREE.LinearFilter;
		hdrJpgEquirectangularMap.generateMipmaps = false;
		hdrJpg.dispose( false );

	} );

image

after

hdrJpg = new HDRJPGLoader( renderer )
        // NEW optional method which allows to specify options
        // including generateMipmaps: true if you are going to use the texture
        // (for example) as an HDR lightmap
        // NOTE: the following line is not needed for the example to work 
        // wrapS/wrapT can be left to THREE.ClampToEdgeWrapping and the background still works
        .setRenderTargetOptions( { wrapS: THREE.RepeatWrapping, wrapT: THREE.RepeatWrapping } )
	.load( 'textures/gainmap/ARHusky_09.jpg', function ( ) {

		hdrJpgPMREMRenderTarget = pmremGenerator.fromEquirectangular( hdrJpg.renderTarget.texture );
		hdrJpgEquirectangularMap = hdrJpg.renderTarget.texture;
		// no need to disable generateMipmaps and specify minFilter
                // they both default to  THREE.LinearFilter
		hdrJpgEquirectangularMap.mapping = THREE.EquirectangularReflectionMapping;
		hdrJpg.dispose( false );

	} );

image

@daniele-pelagatti daniele-pelagatti added bug Something isn't working and removed more info needed Further information is requested labels Nov 29, 2023
daniele-pelagatti added a commit that referenced this issue Nov 29, 2023
…fy renderTarget (and toDataTexture) options

related issues: #14 and #15 

The library will no more generate mipmaps by default but will require to explicitly enable them, also, in general, the library will assume a more 'un-opinionated' stance regarding the output texture options.

BREAKING CHANGE: `generateMipmaps` is no longer `true` by default, both `minFilter` is  no longer `LinearMipMapLinearFilter` by default but `LinearFilter`, `wrapS` and `warpT` are no longer `RepeatWrapping` by default but `ClampToEdgeWrapping`
daniele-pelagatti pushed a commit that referenced this issue Nov 29, 2023
# [3.0.0](v2.0.7...v3.0.0) (2023-11-29)

### Bug Fixes

* **loaders:** properly catches render errors and calls onError callback ([b9bcdd1](b9bcdd1)), closes [#16](#16)

### Features

* **core:** disables default mipmap generation, enables user to specify renderTarget (and toDataTexture) options ([147d278](147d278)), closes [#14](#14) [#15](#15)

### BREAKING CHANGES

* **core:** `generateMipmaps` is no longer `true` by default, both `minFilter` is  no longer `LinearMipMapLinearFilter` by default but `LinearFilter`, `wrapS` and `warpT` are no longer `RepeatWrapping` by default but `ClampToEdgeWrapping`
@daniele-pelagatti
Copy link
Contributor

3.0.0 should fix this, let me know otherwise and I'll re-open it

bestsoftwaretopappreviews30 added a commit to bestsoftwaretopappreviews30/gainmap-js that referenced this issue Sep 12, 2024
# [3.0.0](MONOGRID/gainmap-js@v2.0.7...v3.0.0) (2023-11-29)

### Bug Fixes

* **loaders:** properly catches render errors and calls onError callback ([e38e377](MONOGRID/gainmap-js@e38e377)), closes [#16](MONOGRID/gainmap-js#16)

### Features

* **core:** disables default mipmap generation, enables user to specify renderTarget (and toDataTexture) options ([0c5d740](MONOGRID/gainmap-js@0c5d740)), closes [#14](MONOGRID/gainmap-js#14) [#15](MONOGRID/gainmap-js#15)

### BREAKING CHANGES

* **core:** `generateMipmaps` is no longer `true` by default, both `minFilter` is  no longer `LinearMipMapLinearFilter` by default but `LinearFilter`, `wrapS` and `warpT` are no longer `RepeatWrapping` by default but `ClampToEdgeWrapping`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants