Removal of the layer count restriction#2612
Conversation
|
Could you open a proposal for this PR? Describing what problem you want to resolve and explain your solution. =) |
a424058 to
e963972
Compare
e963972 to
c2a1517
Compare
|
|
||
| rtCache.set(textureSetId, renderTarget); | ||
| } else { | ||
| drawMap(renderTarget, tiles, renderer, extent); |
There was a problem hiding this comment.
If we're caching the render targets, what is the use of redrawing their textures every time we access them? Am I missing something?
There was a problem hiding this comment.
Yes, it's temporary. I'll add a mechanism to update the texture and parent texture when the opacity changes.
|
|
||
| drawMap(renderTarget, tiles, renderer, extent); | ||
|
|
||
| rtCache.set(textureSetId, renderTarget); |
There was a problem hiding this comment.
I was caching array render targets instead of array textures because it was impossible to extract the textures from the RTs and manage their memory separately, but maybe now that you're using simple render targets, this becomes possible and we can just maintain a cache of textures directly, and only use one RT for everything?
There was a problem hiding this comment.
I'm not sure, There might be a problem with the framebuffer linked to the render target
To try
| renderer.setRenderTarget(previousRenderTarget); | ||
| renderer.setClearAlpha(a); | ||
|
|
||
| return renderTarget; |
There was a problem hiding this comment.
The returned value is not used anymore and the documented type is now incorrect
There was a problem hiding this comment.
it's draft PR, it's still evolving, no need for an in-depth review right now
| * @param tiles - An array of RasterTile objects, each containing textures. | ||
| * @param max - The maximum number of layers for the DataArrayTexture. | ||
| * @param type - Layer set identifier: 'c' for color, 'e' for elevation. | ||
| * @param renderer - The renderer used to render textures. |
| uniform vec4 offsetScale; | ||
| uniform float opacity; | ||
|
|
||
| const float CTOYL = 20037508.34 / 3.141592653589793 * 0.69314718; |
There was a problem hiding this comment.
Could you comment what these numbers represent?
There was a problem hiding this comment.
i'll add some comment
- 20037508.34 : max projection in speudo mercator
- 0,69... : Ln(2)
There was a problem hiding this comment.
What do you mean by "max projection"?
| uniforms: { | ||
| // This uniform will be updated with each source 2D texture | ||
| map: { value: null }, | ||
| texture_footprint: { value: new THREE.Vector2() }, |
There was a problem hiding this comment.
Could you comment what these properties represent?
There was a problem hiding this comment.
Footprint means the area/extent covered by something on the ground.
map: map to project on the groundtexture_footprint: texture coverage on groundcamera_footprint: camera coverage visible on groundoffsetScale: the map portion to be projected
c2a1517 to
813670b
Compare
813670b to
92e746a
Compare
743300c to
e40bdb5
Compare
6b4f653 to
b8f2e2e
Compare
b8f2e2e to
cfeace0
Compare
cfeace0 to
42f3068
Compare
42f3068 to
f81e8c3
Compare
f81e8c3 to
416a66e
Compare
416a66e to
3f507b4
Compare
|
@Neptilo @HoloTheDrunk Could you take a look to this PR this week? |
| tile.material.setUniform('geoidHeight', geoidHeight); | ||
|
|
||
| const parent_uniforms = parent.material.uniforms; | ||
| const child_uniforms = tile.material.uniforms; |
There was a problem hiding this comment.
Use camel-case to keep coding style consistent.
There was a problem hiding this comment.
Would you add camel-case in eslint rules ?
There was a problem hiding this comment.
I think it would be good to do it eventually, but for now there would be too many variables to rename (111). In a separate PR, why not.
| const parent_uniforms = parent.material.uniforms; | ||
| const child_uniforms = tile.material.uniforms; | ||
|
|
||
| if (parent.material.uniforms.map.value) { |
There was a problem hiding this comment.
You can use the previously defined parent_uniforms here.
| tile.setBBoxZ({ min: rasterElevationNode.min, max: rasterElevationNode.max, scale: rasterElevationNode.layer.scale, geoidHeight }); | ||
| } else { | ||
| tile.setBBoxZ({ min: parent.obb.z.min, max: parent.obb.z.max, geoidHeight }); | ||
| } |
There was a problem hiding this comment.
An explanatory comment for all this added code would be nice.
|
|
||
| float toY(float lat) { | ||
| float s = sin(lat); | ||
| return 0.5 * log2((1.0 + s) / (1.0 - s)) * CTOYL; |
There was a problem hiding this comment.
Where does this formula come from?
There was a problem hiding this comment.
You could use this opportunity to already write this new file in TypeScript.
|
|
||
| const quad = new THREE.Mesh(geometry, materialUnit); | ||
|
|
||
| export function drawMap( |
There was a problem hiding this comment.
Can you document the function?
| depthBuffer: false, // No depth buffer needed for simple 2D texture copy | ||
| }); | ||
| const arrayTexture = renderTarget.texture; | ||
| const forwarding = (from, to) => ((from == to) ? 'transformUnit' : `${from} => ${to}`); |
There was a problem hiding this comment.
Can you document what materials contains and give forwarding a more explicit name? For example, "makeProjectionId" if you think that makes sense.
There was a problem hiding this comment.
Could you document what the purpose of these materials is? As I understand it: running projection computations on the GPU?
3f507b4 to
60ba0ac
Compare
dfe65fd to
837c806
Compare
ea09d17 to
1033823
Compare
d552ff1 to
3f4a194
Compare
2279834 to
0a88be2
Compare
a306bb9 to
6170906
Compare
2fec763 to
dd87cfe
Compare
BREACKING CHANGES: remove pdateLayeredMaterialNodeImagery, updateLayeredMaterialNodeElevation
… if raster tiles aren't loaded
WARNING : There’s still unresolved bugs:
Description
Replace all textures array by just 2 textures, map and displacement map. All array textures are flatten in these maps.
Motivation and Context
LayeredMaterialwith any Three.js materials