kml/Collada export of appearanes with more than one theme fails about importer-exporter HOT 14 CLOSED

Thanks for reporting the issue. Looking forward to your fix. Please create a pull request, or send a patch file. Alternatively, if this is more convenient for you, simply post the adapted Java file.

Cheers,
Claus

from importer-exporter.

from importer-exporter.

...new attempt of attaching a patch...

patch0.zip

from importer-exporter.

...and now I believe I managed to do a "pull request". I'm pretty new to github, having only used git for internal source management previously :-)

from importer-exporter.

Are you able to also share a dataset for testing?

from importer-exporter.

Im not sure if it will help actually. Any dataset with two appearance themes, where the surface geometries are connected to two surface_data records (one for each appearance theme) and only colors are used for the surface material (no textures) may trigger this bug. When executing the query in "COLLADA_GEOMETRY_AND_APPEARANCE_FROM_ROOT_ID" or "COLLADA_IMPLICIT_GEOMETRY_AND_APPEARANCE_FROM_ROOT_ID" each surface_geometry will produce two rows in the result listing, one for each appearance theme the surface is connected to.

If the first row has the selected theme the material data is first read correctly and inserted into the x3dmaterial table for the given surface id. But when the second row is read (the row that does not have the selected theme) the x3dmaterials table for this surface id is overwritten with default material data.

If however the row with the non-selected theme is read first the x3dmaterials table will initially get default data, but is then overwritten with the correct material when the second row is read with data for the selected theme, and the result is then correct.

So the bug hits rather arbitrarily, depending on how the data is read from the database rather than on some specific dataset. In my tests the same data could generate databases that where read correctly or not depending on the assignment of GIDs (that again may determine the sequence of the rows read)!

In my fix I have added some tests so that data in the x3dmaterials table is overwritten only with data from the selected theme. Default material data is only written to the x3dmaterials table if there is no data already there for the given surface id.

from importer-exporter.

@clausnagel sounds a little familiar to an issue we once had with the Berlin data. It could be that we have fixed it for the vcDB. I remember when updating the KML/Collada exporter code that we had a discussion about appearances.

from importer-exporter.

@runaas Thanks for your comprehensive description. I created a test dataset based on the Gasometer dataset shipped with the importer/exporter. It contains 430 LOD2 buildings in Berlin. For each building I added three appearance themes red, green and blue assigning the same color. The EPSG code for this dataset is 3068.

issue_24.zip

I re-imported the dataset several times and did several COLLADA exports after each import, but could not reproduce the bug so far. I will continue testing. Could you please verify whether my dataset fits your bug description and whether you can reproduce the bug with it?

@yaozhihang Could you also please check the bug and fix?

@FxKu Yes, you are right. I could neither reproduce the bug with the vcDB. I will also continue testing with the vcDB.

from importer-exporter.

Ok, I spent more time and managed to reproduce the bug. In the test dataset issue_24.zip from my previous post, the target of each X3DMaterial appearance is always the root gml:MultiSurface geometry of the boundary surfaces. I changed this for the bldg:WallSurface of one building for which the appearances now target the separate gml:Polygon surface members of the gml:MultiSurface instead. The following dataset contains this single building.

issue_24_2.zip

When exporting this building to COLLADA, some polygons of the bldg:WallSurface receive the color of the chosen theme and some are assigned the default wall color as reported by @runaas.

I applied the proposed patch and it fixed the issue.

@yaozhihang Could you please confirm and merge?

@FxKu We actually do not have this bug in the vcDB as you supposed. However, our code substantially differs so that we cannot easily port our solution.

from importer-exporter.

Ah! Great!

And this teaches us all (also me) that my data is not necessarily similar to your data. :-)

from importer-exporter.

@runaas @clausnagel @FxKu , Many thanks for testing and confirming the bug.
I'll check the pull request and merge it into the master branch as soon as possible.

from importer-exporter.

@runaas Yeah, that's why test datasets are always important for us.

from importer-exporter.

This should be fixed by now as parts of vcDB have been merged with master. Correct @clausnagel ?

from importer-exporter.

Yes. It should have been solved for the current version as well, but obviously the issue was never closed. So I close it now.

from importer-exporter.