copyTextureToTexture sourcebox, flipY from source #21986

mbredif · 2021-06-14T14:39:50Z

Related issue: #21942

Description

This PR aligns the apis of copyTextureToTexture and copyTextureToTexture3D in the WebGLRenderer :

copyTextureToTexture was missing a source box parameter (only full images could be copied)
a source level is added to enable copying a higher level than 0 from the source image for advanced uses.
the parameters are reordered in both functions as ( srcTexture, dstTexture, dstPosition = undefined, srcBox = undefined, dstLevel = 0, srcLevel = 0 ) as all parameters but textures have sensible defaults (destination position at origin, full source box and levels at 0).
usage of the previous api is detected by checking if the first argument is a texture, to reorder the parameters and print a deprecation warning.

examples/webgl_materials_texture_partialupdate.html

it now demonstrates copying sub parts of an input 2D texture

The box is reduced so that it points to valid pixels in both the source and destination textures. As an alternative, this code could be simplified if this burden is given to the user (big boxes will trigger webgl error).

finally, the UNPACK parameters are now taken from the source texture rather than the destination texture. It makes more sense in my use cases (otherwise I always end up at least copying flipY and unpackAlignment from the srcTexture to the dstTexture before the copy as a workaround)

mrdoob · 2021-06-16T18:24:55Z

/cc @DavidPeicho

DavidPeicho

At this point those methods are so complex than I am wondering if they shouldn't even be merged as a single one...
The only notable differences to me are:

The depth component
The unpacking
The name of the function to upload the data

I think we can work a bit on simplifying the function while keeping the features added.

DavidPeicho · 2021-06-16T20:23:44Z

src/renderers/WebGLRenderer.js

+		if ( ! srcTexture.isTexture ) {
+
+			console.warn( 'THREE.WebGLRenderer: .copyTextureToTexture() now expects (srcTexture, dstTexture, dstPosition, srcBox, dstLevel, srcLevel).' );
+			return this.copyTextureToTexture( arguments[ 1 ], arguments[ 2 ], arguments[ 0 ], undefined, arguments[ 3 ] );


Why do you use arguments here instead of the named parameters?

arguments are out of order, so the named parameters would be more confusing IMHO this.copyTextureToTexture(dstTexture, dstPosition, srcTexture, undefined, srcBox)

DavidPeicho · 2021-06-16T20:29:58Z

src/renderers/WebGLRenderer.js

+
+		}
+
+		const image = srcTexture.mipmaps[ srcLevel ] || ( srcLevel == 0 ? srcTexture.image : null );


Hum, how should we differentiate base texture from mipmap 0? @mrdoob
Here what's happening is a bit weird to me

I agree that I only guessed from code that textures either had mipmaps or a single image (which would act as mipmap level 0). Is that true ? could a texture have both ? could image refer to a mipmap level different than 0 ?

That makes sense actually. If you have mipmaps, level 0 should be the normal textures... But that's confusing

DavidPeicho · 2021-06-16T20:38:20Z

src/renderers/WebGLRenderer.js

+
+		}
+
+		let skipPixels = srcBox ? srcBox.min.x : 0;


Not too important, but here we can initialize all variables to the default value, and then perform the condition once:
srcBox !== undefined.

true. just tell me which coding style you prefer in three.

DavidPeicho · 2021-06-16T20:40:23Z

src/renderers/WebGLRenderer.js

+		let y = dstPosition ? dstPosition.y : 0;
+
+		// ensure skipPixels, skipRows, x and y >= 0
+		if ( skipPixels < 0 || x < 0 ) {


I think both of those conditions aren't needed and should work the same way srcBox is clamped to 0 when negative.
They add a lot of code for something the user should clearly not do. We can add a little disclaimer in the doc

this code selects the biggest valid box inside the srcBox for copy : srcbox is shrunk so that it is contained in the source texture and the dstBox ={ min: dstPosition, max = dstPosition-srcBoxmin+srcBox.max} is contained in the destination texture.

as said in the PR, the alternative is to pass directly srcBox and dstPosition to webGL and let it crash if it is not valid. Both are fine to me, but the one we chose should be documented.

DavidPeicho · 2021-06-16T20:41:54Z

src/renderers/WebGLRenderer.js

+
+		}
+
+		const width = Math.min( dstTexture.image.width - x, srcBox ? Math.min( image.width, srcBox.max.x + 1 ) - skipPixels : image.width );


If we decide to remove the above two conditions, then we also need to update that part because it will not be valid anymore

Indeed, they will simplify : const width = srcBox.max.x - srcBox.min.x + 1;

mbredif

Thanks @DavidPeicho for the review. I gave your ideas a try, merging the two functions notably : dev...mbredif:copyTextureToTexture_merged .
Is that what you had in mind ? @mrdoob and @Mugen87, do you prefer this second approach ?

mbredif · 2021-06-17T14:58:45Z

src/renderers/WebGLRenderer.js

+		if ( ! srcTexture.isTexture ) {
+
+			console.warn( 'THREE.WebGLRenderer: .copyTextureToTexture() now expects (srcTexture, dstTexture, dstPosition, srcBox, dstLevel, srcLevel).' );
+			return this.copyTextureToTexture( arguments[ 1 ], arguments[ 2 ], arguments[ 0 ], undefined, arguments[ 3 ] );


arguments are out of order, so the named parameters would be more confusing IMHO this.copyTextureToTexture(dstTexture, dstPosition, srcTexture, undefined, srcBox)

mbredif · 2021-06-17T15:01:42Z

src/renderers/WebGLRenderer.js

+
+		}
+
+		const image = srcTexture.mipmaps[ srcLevel ] || ( srcLevel == 0 ? srcTexture.image : null );


I agree that I only guessed from code that textures either had mipmaps or a single image (which would act as mipmap level 0). Is that true ? could a texture have both ? could image refer to a mipmap level different than 0 ?

mbredif · 2021-06-17T15:02:36Z

src/renderers/WebGLRenderer.js

+
+		}
+
+		let skipPixels = srcBox ? srcBox.min.x : 0;


true. just tell me which coding style you prefer in three.

mbredif · 2021-06-17T15:08:31Z

src/renderers/WebGLRenderer.js

+		let y = dstPosition ? dstPosition.y : 0;
+
+		// ensure skipPixels, skipRows, x and y >= 0
+		if ( skipPixels < 0 || x < 0 ) {


this code selects the biggest valid box inside the srcBox for copy : srcbox is shrunk so that it is contained in the source texture and the dstBox ={ min: dstPosition, max = dstPosition-srcBoxmin+srcBox.max} is contained in the destination texture.

as said in the PR, the alternative is to pass directly srcBox and dstPosition to webGL and let it crash if it is not valid. Both are fine to me, but the one we chose should be documented.

mbredif · 2021-06-17T15:11:38Z

src/renderers/WebGLRenderer.js

+
+		}
+
+		const width = Math.min( dstTexture.image.width - x, srcBox ? Math.min( image.width, srcBox.max.x + 1 ) - skipPixels : image.width );


Indeed, they will simplify : const width = srcBox.max.x - srcBox.min.x + 1;

Mugen87

When I initially added copyTextureToTexture(), it was important to me to cover the most basic use cases and keep the method simple. Now it is enhanced to cover more advanced use cases but that makes the implementation as well as the signature more complex.

TBH, as long as there is no more demand for such features I would prefer to keep the method simple. And let advanced users monkey-patch the method if necessary.

mbredif · 2021-06-18T06:12:30Z

What about this version ? https://github.com/mbredif/three.js/blob/copyTextureToTexture_merged/src/renderers/WebGLRenderer.js#L2083-L2227 . As @DavidPeicho mentioned, the 2 copy functions could be merged to factor common code. I also removed the box shrinking code to simplify the implementation.

DavidPeicho · 2021-06-18T09:04:45Z

I quite like the second version! Thanks!

It's indeed a bit of code, but that's the issue with abstracting a method that has so many options.

Mugen87 · 2024-06-19T13:22:38Z

The most important part of this PR (copying sub parts of an input 2D texture) was implemented in #28281. I would leave the other features out for now and only add them if they are frequently requested.

mbredif added 2 commits June 14, 2021 16:19

copyTextureToTexture sourcebox flipY

e6b3ca2

doc

0310fbf

mrdoob requested a review from Mugen87 June 16, 2021 18:24

mrdoob added this to the r130 milestone Jun 16, 2021

DavidPeicho reviewed Jun 16, 2021

View reviewed changes

DavidPeicho suggested changes Jun 16, 2021

View reviewed changes

mbredif commented Jun 17, 2021

View reviewed changes

Mugen87 reviewed Jun 17, 2021

View reviewed changes

mrdoob modified the milestones: r130, r131 Jun 30, 2021

mrdoob modified the milestones: r131, r132 Jul 28, 2021

mrdoob modified the milestones: r132, r133 Aug 26, 2021

mrdoob modified the milestones: r133, r134 Sep 30, 2021

mrdoob modified the milestones: r134, r135 Oct 28, 2021

mrdoob modified the milestones: r135, r136 Nov 26, 2021

mrdoob modified the milestones: r136, r137 Dec 24, 2021

mrdoob modified the milestones: r137, r138 Jan 26, 2022

mrdoob modified the milestones: r138, r139 Feb 23, 2022

mrdoob removed this from the r139 milestone Mar 24, 2022

Mugen87 modified the milestones: r153, r154 Jun 1, 2023

mrdoob modified the milestones: r154, r155 Jun 29, 2023

mrdoob modified the milestones: r155, r156 Jul 27, 2023

mrdoob modified the milestones: r156, r157 Aug 31, 2023

mrdoob modified the milestones: r157, r158 Sep 28, 2023

mrdoob modified the milestones: r158, r159 Oct 27, 2023

mrdoob modified the milestones: r159, r160 Nov 30, 2023

mrdoob modified the milestones: r160, r161 Dec 22, 2023

mrdoob modified the milestones: r161, r162 Jan 31, 2024

mrdoob modified the milestones: r162, r163 Feb 29, 2024

mrdoob modified the milestones: r163, r164 Mar 29, 2024

mrdoob modified the milestones: r164, r165 Apr 25, 2024

mrdoob modified the milestones: r165, r166 May 31, 2024

Mugen87 closed this Jun 19, 2024

Mugen87 modified the milestone: r166 Jun 19, 2024


		}

		const image = srcTexture.mipmaps[ srcLevel ] \|\| ( srcLevel == 0 ? srcTexture.image : null );


		}

		const width = Math.min( dstTexture.image.width - x, srcBox ? Math.min( image.width, srcBox.max.x + 1 ) - skipPixels : image.width );

Uh oh!

copyTextureToTexture sourcebox, flipY from source #21986

copyTextureToTexture sourcebox, flipY from source #21986

Uh oh!

Conversation

mbredif commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Jun 16, 2021

Uh oh!

DavidPeicho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbredif left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mugen87 left a comment

Choose a reason for hiding this comment

Uh oh!

mbredif commented Jun 18, 2021

Uh oh!

DavidPeicho commented Jun 18, 2021

Uh oh!

Mugen87 commented Jun 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mbredif commented Jun 14, 2021 •

edited

Loading

DavidPeicho left a comment •

edited

Loading