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

Update ConvertType #23635

Merged
merged 41 commits into from
Mar 22, 2022
Merged

Update ConvertType #23635

merged 41 commits into from
Mar 22, 2022

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Mar 3, 2022

Related issue: #23601 (comment)

Description

  • Remove valueComponents.
  • Add arguments' flatting.
  • Remove valueClass.
  • Add cache.
  • Rework ConvertType.
  • Fix uint generation in NodeBuilder.

@LeviPesin
Copy link
Contributor Author

Actually, here can be reused function getValueFromType() from #23663 to remove the valueClass parameter.

Also I suggest adding to the ConvertType some cache - so e.g. if float(0) would be called hundred times it will return the same value. This cannot be done with WeakMap, because it only accepts keys which are objects, so maybe it should be done with just Map.

@LeviPesin LeviPesin changed the title Remove valueComponents, add arguments' flatting Update ConvertType Mar 12, 2022
@LeviPesin
Copy link
Contributor Author

@sunag What is your opinion about something like vec4(1.0, new Vector2(1.0, 1.0), 1.0)? Should it be supported or rather not?

@sunag
Copy link
Collaborator

sunag commented Mar 13, 2022

@sunag What is your opinion about something like vec4(1.0, new Vector2(1.0, 1.0), 1.0)? Should it be supported or rather not?

Sounds good!

Can you do this update without change src(core) code?

@LeviPesin
Copy link
Contributor Author

This is the part that I called arguments' flatting. I changed the core code to remove valueComponents - I can revert it back.

@LeviPesin
Copy link
Contributor Author

Done.

@LeviPesin LeviPesin marked this pull request as ready for review March 13, 2022 07:59
@sunag
Copy link
Collaborator

sunag commented Mar 15, 2022

Can you check the webgpu_nodes_playground example? Seems that some vectors contains NaN values.

image

@sunag sunag added this to the r139 milestone Mar 15, 2022
@LeviPesin
Copy link
Contributor Author

I've fixed the error, but for some reason the models now appear to be white instead of black...

@sunag
Copy link
Collaborator

sunag commented Mar 19, 2022

@LeviPesin Seems that vec*() no longer works with scalar values.

See ColorSpaceNode.LinearTosRGB:

const a = sub( mul( pow( value.rgb, vec3( 0.41666 ) ), 1.055 ), vec3( 0.055 ) );
const b = mul( rgb, 12.92 );
const factor = vec3( lessThanEqual( rgb, vec3( 0.0031308 ) ) );

const rgbResult = mix( a, b, factor );

return join( rgbResult.r, rgbResult.g, rgbResult.b, value.a );
nodeVar4 = pow( DiffuseColor.xyz, vec3<f32>( 0.0, 0.0, 0.0 ) );
nodeVar5 = ( nodeVar4 * vec3<f32>( 1.055 ) );
nodeVar6 = ( nodeVar5 - vec3<f32>( 0.0, 0.0, 0.0 ) );
nodeVar7 = ( DiffuseColor.xyz * vec3<f32>( 12.92 ) );
nodeVar8 = mix( nodeVar6, nodeVar7, vec3<f32>( 0.0, 0.0, 0.0 ) );
Output = vec4<f32>( nodeVar8.x, nodeVar8.y, nodeVar8.z, DiffuseColor.w );

@LeviPesin
Copy link
Contributor Author

Fixed.

@LeviPesin
Copy link
Contributor Author

Is it better now?

Also, maybe it would be better to remove PI, PI2, PI_HALF, RECIPROCAL_PI, RECIPROCAL_PI2 (but keep EPSILON and add INFINITY = 1 / EPSILON)? Just something like Math.PI or float( Math.PI ) could be used instead when needed - and it will cache.

Also, the cache should be added to the new ConstNode( Number( prop ), 'uint' ) and new ShaderNodeObject( new ConstNode( obj ) );, I think.

@sunag
Copy link
Collaborator

sunag commented Mar 21, 2022

Also, maybe it would be better to remove PI, PI2, PI_HALF, RECIPROCAL_PI, RECIPROCAL_PI2 (but keep EPSILON and add INFINITY = 1 / EPSILON)? Just something like Math.PI or float( Math.PI ) could be used instead when needed - and it will cache.

@LeviPesin I think we can keep as is for now...

@sunag sunag merged commit 3b5ad88 into mrdoob:dev Mar 22, 2022
@sunag
Copy link
Collaborator

sunag commented Mar 22, 2022

Thanks! 👍

@LeviPesin LeviPesin deleted the patch-1 branch March 22, 2022 17:03
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Flat params

* Remove valueComponents

* Lint

* Lint

* Reuse functions from InputNode.js

* Fix

* Fix

* Add cache

* Move ArrayMap and flatArray

* Revert valueComponents' removal

* Fix ArrayMap

* Fix

* Fix

* Fix

* Add size cap to ArrayMap

* Fix

* Rework ConvertType

* Fix

* Another fix

* Fix uints in NodeBuilder

* Change ArrayMap cache to just Map

* optional params

* fix color type

* fix uniform

* recovering bad fix conflict

* static cacheMap

* cleanup

* Two cache maps

* Cleanup

* Fix

* Add constants' cache to ShaderNodeObject

* Fix

* Simplify cache

* Fix

* Fix typo

* fix negate cache

* cleanup

Co-authored-by: sunag <[email protected]>
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