-
Notifications
You must be signed in to change notification settings - Fork 303
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
refactor(LayeredMaterial): migrate to TypeScript #2477
base: master
Are you sure you want to change the base?
refactor(LayeredMaterial): migrate to TypeScript #2477
Conversation
c4d4b46
to
d283702
Compare
Hi, yes this is still a WIP and I'm tracking down the source of these issues. |
6c2231f
to
c46772f
Compare
b237291
to
8e0e7d4
Compare
Squashed commits (oldest first): - fix(tests): add new empty methods to mock Material - refactor: use interface where possible - fix: use setUniform for material showOutline set - fix(elevation): correct antilogy - refactor: rename layer to tile where relevant - fix(geoidlayer): revert wrong uniform access change
7baae33
to
46b6bf9
Compare
a7cce77
to
46b6bf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! I made some suggestions while debugging/reviewing the code.
fillInProp(defines, 'NUM_VS_TEXTURES', nbSamplers[0]); | ||
fillInProp(defines, 'NUM_FS_TEXTURES', nbSamplers[1]); | ||
// TODO: We do not use the fog from the scene, is this a desired | ||
// behavior? | ||
fillInProp(defines, 'USE_FOG', 1); | ||
fillInProp(defines, 'NUM_CRS', crsCount); | ||
|
||
initModeDefines(defines); | ||
fillInProp(defines, 'MODE', RenderMode.MODES.FINAL); | ||
|
||
// @ts-expect-error: global constexpr | ||
fillInProp(defines, 'DEBUG', +__DEBUG__); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would it be better to just assign the properties directly into defines
. This would remove the need for type assertions and ease the readability of the code. Something like
const defines: typeof this.defines = {
NUM_VS_TEXTURES: nbSamplers[0],
NUM_FS_TEXTURES: nbSamplers[1],
USE_FOG: 1,
NUM_CRS: crsCount,
initModeDefines(),
MODE: RenderMode.MODES.FINAL,
DEBUG: __DEBUG__,
};
this.defines = defines;
You would need to change the initNodeDefines
to output the key-mapped object for both ELEVATION_MODES
and RenderModes.MODES
. Such mapping could be done with
Object.fromEntries(
Object.entries(ELEVATION_MODES).map(([key, value]) => [`ELEVATION_${key}`, value])
) as { [K in keyof typeof ELEVATION_MODES as `ELEVATION_${K}`]: typeof ELEVATION_MODES[K] }
P.S.: I didn't find a way to do such mapping without explicit casts
P.P.S: Forgot to use DefineMapping
in my suggestion
|
||
public layersNeedUpdate: boolean; | ||
|
||
public override defines: LayeredMaterialDefines; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not adding an override for uniforms
too?
Something like MappedUniforms<LayeredMaterialRawUniforms>
? uniforms
would be correctly typed in the whole module.
outlineColors.push(new THREE.Color(1.0, 0.5, 0.0)); | ||
} | ||
|
||
this.initUniforms({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same vein as my suggestion for defines
, why not have a single call to initUniforms
which could return all uniforms and assign the uniforms
property directly?
@HoloTheDrunk You could fix the zoom and picking bug by correctly assigning the material.setUniform('objectId', this.id);
// instead of
material.objectId = this.id; |
Description
Converted the
LayeredMaterial
class and eponymous file to TypeScript.Took this opportunity to do some internal cleanup and refactoring of things like the way the layers (or more precisely
RasterTile
s) were stored, the uniforms and defines, etc... This means the internal API for the material has changed since we now keepRasterColorTile
s and the oneRasterElevationTile
separate internally. A conveniencegetLayer
method has been provided matching the previous API for cases where the exact type ofRasterTile
is not important.Whether we should rename the "layer" fields to "tile" to match the actual data type is up for debate. I'm all for it, but I'm not aware of how dependent people might be on those names.
Also planning on making the new uniforms/defines convenience TS-specific functions available to the
PointsMaterial
class and nukingsrc/Renderer/CommonMaterial.js
out of existence in a coming PR.Motivation and Context
This PR fits into the larger motion to migrate iTowns to TypeScript.
Tested on Ubuntu 22.04.5 LTS x86_64 using Firefox 126.0.1 (64-bit)