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

kml/Collada export of appearanes with more than one theme fails #24

Closed
runaas opened this issue Feb 1, 2017 · 14 comments
Closed

kml/Collada export of appearanes with more than one theme fails #24

runaas opened this issue Feb 1, 2017 · 14 comments

Comments

@runaas
Copy link

runaas commented Feb 1, 2017

When CityGML objects have more than one named appearance theme the material data from the surface_data can be overwritten with default colors. I have a fix that I'll check in as soon as I find out how...

@clausnagel
Copy link
Member

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

@runaas
Copy link
Author

runaas commented Feb 3, 2017 via email

@runaas
Copy link
Author

runaas commented Feb 3, 2017

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

patch0.zip

@runaas
Copy link
Author

runaas commented Feb 6, 2017

...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 :-)

@runaas runaas closed this as completed Feb 6, 2017
@runaas runaas reopened this Feb 6, 2017
@clausnagel
Copy link
Member

Are you able to also share a dataset for testing?

@runaas
Copy link
Author

runaas commented Feb 6, 2017

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.

@FxKu
Copy link
Member

FxKu commented Feb 6, 2017

@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.

@clausnagel
Copy link
Member

@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.

@clausnagel
Copy link
Member

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.

@runaas
Copy link
Author

runaas commented Feb 7, 2017

Ah! Great!

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

@yaozhihang
Copy link
Member

@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.

@clausnagel
Copy link
Member

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

@FxKu FxKu added this to the v4.0 milestone Feb 6, 2018
@FxKu
Copy link
Member

FxKu commented Feb 19, 2018

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

@clausnagel
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants