loicgasser / quantized-mesh-tile

Quantized-Mesh encoder/decoder and topology builder
MIT License
90 stars 20 forks source link

Correct Lighting #21

Closed thiloSchlemmer closed 4 years ago

thiloSchlemmer commented 6 years ago

Hi @loicgasser ,

i used successfully your library to create terrain-files from my mesh-data. Thanks a lot for this great tool!

Unfortunately is the shading in cesium not correct. In https://github.com/loicgasser/quantized-mesh-tile/issues/6 you mentioned, that lighting is not correct yet.

After some stumbling around in the code i've got following question: Why you use wheightedNormals in computeNormals (utils.py line 185)?

According to the code in Cesium's GeometryPipeline it is not neccessary to get the vertex-normals.

for i in xrange(0, numFaces): face = faces[i] for j in face: normalsPerVertex[j] = c3d.add(normalsPerVertex[j], normalsPerFace[i])

instead of:

for i in xrange(0, numFaces): face = faces[i] weightedNormal = [c * areasPerFace[i] for c in normalsPerFace[i]] # alternative? #weightedNormal = c3d.multiplyByScalar(normalsPerFace[i],areasPerFace[i]) for j in face: normalsPerVertex[j] = c3d.add(normalsPerVertex[j], weightedNormal)

loicgasser commented 6 years ago

Hi. Here is a technical explanation for weighted vertex normals on vertices and its influence on shading. http://www.bytehazard.com/articles/vertnorm.html But yes it is possible to add all the normal vector of the faces attached to a vertex and normalize the resulting vector. But this is not really the issue here. This code is Work in progress. There is some code I could remove which would generate some ok looking normals.

The main problems is: Context: neighbouring tile(s) and their influence on vertex normals located on the edge of the tile.

So if vertex normals where to be computed during a preprocessing step, I could add a slot so that you explicitely input them during the tile creation. Maybe I missing something or taking this problem the wrong way.

thiloSchlemmer commented 6 years ago

Hi,

many thanks for the hint and your explanations.

I currently managed my first issue with the normals (adding extension in layer.json) and can now follow the main problem as you mentioned.

Are the normal-vectors on the tile-edges really a problem in all circumstances? To illustrate my question, i created obj-files from my tiled tin-data and visualized them in meshlab. The normals on the tile-edges looks ok for me. Did i miss something? tiled_tin Your library is one part in my processing chain, where neighbouring tiles are hard to reach:

Thread (one Thread per tile):

- [DEM as TIFF] > clip_dem > [DEM as ASCII-GRD]
- [DEM as ASCII-GRD] > grid2tin > [TIN as WKT]
- [TIN as WKT] > quantized_mesh_tile > [Tile as terrain]

I would give the "ok looking normals" a try. If you give me a hint, where code should be removed, then i can test it out.

thiloSchlemmer commented 6 years ago

Hi,

may be i found the problem...

in Line 557@terrain.py are padding bytes are inserted.

if i log in Cesium the oct-encoded normals, then this trailing bytes are part of it [1,1,147,143,...] Result in my terrain: padding_bytes_before

After moving this padding bytes after oct-encoded normals, result in my terrain: padding_bytes_after

code changes to (at Line 562):

   f.write(packEntry(meta['extensionLength'], 2 * vertexCount))

        metaV = TerrainTile.OctEncodedVertexNormals
        for i in xrange(0, vertexCount - 1):
            x, y = octEncode(self.vLight[i])
            f.write(packEntry(metaV['xy'], x))
            f.write(packEntry(metaV['xy'], y))

        # Add 2 bytes of padding
        f.write(packEntry('B', 1))
        f.write(packEntry('B', 1))

I dont know much about the concept of padding, so this solution is a product of trial and error :) In the specs i found only for the index data the requirement of padding. So i tried to remove it completely, but thats not worked. The tile was corrupt.

I hope, you can reproduce the results and confirm to this solution.

loicgasser commented 6 years ago

Looks promising and this is exciting. Thanks a lot!

Since light extension is currently not working it doesn t really matter :D I ll mark it as experimental.

PR is welcome and Il give it a try.

Nothing is being said about extension headers indeed, but for some reason I noticed you needed some padding as well and never tried to put it after.

From doc:

To enforce proper byte alignment, padding is added before the IndexData to ensure 2 byte alignment for IndexData16 and 4 byte alignment for IndexData32.

loicgasser commented 6 years ago

Mmm it seems I am not inserting padding anywhere else.

Have you tried to remove it from there and insert it before the index data instead?

thiloSchlemmer commented 6 years ago

Nope,

I thought, If my Mesh is displayed correctly, then it is better to avoid changes there and concentrate on all following statements.

I will give it a try in the next week on my test-machine...

thiloSchlemmer commented 6 years ago

...nur i noticed, that the later padding creates little dots in the Material. Same effect showed up, If i Set all normals to unit vector of [0,0,1]

loicgasser commented 6 years ago

Cool. I will also test it in a week or 2. Receiving new terrain data soon so I ll keep you up to date.

thiloSchlemmer commented 6 years ago

Hi,

after a quick search in Cesium source code: At this line the CesiumTerrainProvider take care of any padding.

No other line of code seems to me relevant for any padding. I think padding is only needed for indexdata and should have no effect after the normals.

But... at this moment i'm confused, why terrain is corrupt without the padding after the oct-encoded normals and works with the padding (but with the beforementioned little dots).

loicgasser commented 6 years ago

Padding is used to provide an efficient memory alignment of the binary data. (the way it is accessed in computer memory)

when working with struct in C which is what is wrapped at python level in this code, you have to take that into account. There is a way to determine it, but so far I always trusted what was in the spec. Just saw your pr

thiloSchlemmer commented 6 years ago

Hi Loic,

a suggestion for the normals on the tile-edges, based on a new worker/helper-class/script:

Hypothesis

To harmonize the normals on the tile-edges, you iterate only over the vertices of a defined edge and adding the normals-value with the neighbouring normals-value. To define the currently edge to harmonize, the worker-class/script have eight methods (instantiated with center_tile):

In the next days i try to make a prototype based upon this suggestion.

loicgasser commented 6 years ago

Interseting idea. Alternatively we could provide all the triangle intersecting a tile, clip using the tile extent and take into account the triangles on the tile edges to compute west/east/north/south unit vectors.

thiloSchlemmer commented 6 years ago

Hi Loic,

a (actually not so) short feedback from my tests with my above mentioned idea...

  1. my Hypothesis smashed against the wall of reality :D ...because vertices on connected-edges between neighbouring tiles MUST not correspond, this is a disadvantage of my tin-production process, where the data of a tile comes directly from cutted dem-data, each tile is produced indepedently from any other
  2. so i must have implement more a 'tile-stitcher' than a 'tile harmonizer'
  3. tile-stitching and harmonizing normals (and also only harmonizing normals ) are working in my prototype, but...
  4. the visual improvement on tile edge normals is limited, altough tile-edge geometry fits now better
  5. additionally i tried to extend the calculation of the weighted normals: i included in the calculation the angle on the vertex.....without any visual improvements
  6. in my tests with my data i stumbled over TerrainTile.eastI,westI,southI,northI where not all Edge-Vertices are listed, it works for me after changing to TerrainTile.eastI = [i for i, x in enumerate(self.u) if x == MAX] . I try to build a unittest in the next weeks which pointing my problem out

In conclusion i guess your above described alternative is the next step, but hard to achieve with my current setup...

loicgasser commented 6 years ago

Hi, thank you for the tests.

in my tests with my data i stumbled over TerrainTile.eastI,westI,southI,northI where not all Edge-Vertices are listed, it works for me after changing to TerrainTile.eastI = [i for i, x in enumerate(self.u) if x == MAX] . I try to build a unittest in the next weeks which pointing my problem out

This should be handled here

thiloSchlemmer commented 6 years ago

Hi,

in the end i stick with my TileStitcher, some improvements and resolved bugs produce the following results...

Normals before stitching tilestitcher_area1_normalscorrectionsa Normals after stitching tilestitcher_area1_normalscorrectionsb

Gaps before stitching tilestitcher_area2_edgecorrectionsa Gaps after stitching tilestitcher_area2_edgecorrectionsb

Because the stitcher (and the attached classes) depends heavily on quantized_mesh_tile API, i would appreciate to make a pull request. But it can also stand alone.

loicgasser commented 6 years ago

Hi sorry I missed that. Yes a pull request would be appriciated. How does it impact performances?

thiloSchlemmer commented 6 years ago

Hi,

at the moment it is clearly a overhead, which should only started, if the normal results of quantized-mesh comes with gaps and degenerated normals on the edges. On my maschine it takes between 0.9 - 2 seconds to stitch three neighbour-tiles together (center, south and south-east), with ca. 4000 vertices in each tile. For now it is only usable as an explicit and optional post-processing step, as the neighbour-tiles must exist beforehand.

After some updates, to get Travis happy, i'm able to create a pull request. Do you want it in a extra branch?

loicgasser commented 6 years ago

Yes that is quite slow and it would add a lot of code. On the other hand I understand it could fit very well in a workflow where you first cut through a existing mesh to create and tiles and then convert them individually. I am not sure about it yet. I saw that you implemented get_edge_vertices extending the TerrainTile class, why look through all the vertices when you can use self.westI, self.southI and so on. Any reason I am missing? Could be faster.

thiloSchlemmer commented 6 years ago

Hi Loic,

i wrote EditableTerrain.get_edge_vertices() in a early stage, where i run in a problem with missed vertices . At this moment i used tests, where i created terraintiles from a wkt-stream->topology and i thought the issue came from comparing floats (terrain.py around line 710). So i want to use the integer-values of the vertices, where the floatingpoint-precision-problem doesn't exist.

In the last hour i tried to write a short test for the (hypothetically) issue, but i cannot reproduce the problem for now. So you're right... self.westI etc. are much faster and i will change this part.

loicgasser commented 6 years ago

Actually I wanted to change that because it could cause issues. When comparing floats you should always use a tolerance. https://www.python.org/dev/peps/pep-0485/#proposed-implementation http://realtimecollisiondetection.net/blog/?p=89

thiloSchlemmer commented 6 years ago

if you want to change that, why not using the concept behind EditableTerrain.get_edge_vertices(). Not iterating and checking floats but filter the relative integer-values from the u-v-arrays. the incoming data maybe comes along with a valid tolerance value (working unit property in oracle spatial or esri geodatabase?), but thats not relevant, if all coordinates are transformed to the barycentric coordinates of the terrain tile, i believe

just my two cents :)

I forgot...

i checked again EditableTerrain.py and found, that get_edge_vertices is needed anyway, as i potentially insert points and self.westI etc. becomes invalid.

thiloSchlemmer commented 6 years ago

hi loic,

some good news. i did some profiling on tileStitcher etc. and found/resolve a bottleneck. At the end the time for stitching (load-stitch-save) three tiles drops from 2 to ca. 0.5 seconds. Sounds that better?