-
Notifications
You must be signed in to change notification settings - Fork 56
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
Comments
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, |
Patch attached. Hopefully it went through?
Rune Aasgaard
Fra: Claus Nagel [mailto:notifications@github.com]
Sendt: 2. februar 2017 18:34
Til: 3dcitydb/importer-exporter <importer-exporter@noreply.github.com>
Kopi: Rune Aasgard <rune.aasgaard@norkart.no>; Author <author@noreply.github.com>
Emne: Re: [3dcitydb/importer-exporter] kml/Collada export of appearanes with more than one theme fails (#24)
Thanks for reporting the issue. Looking forward to your fix. Please create a pull request<https://gist.github.com/Chaser324/ce0505fbed06b947d962>, or send a patch file. Alternatively, if this is more convenient for you, simply post the adapted Java file.
Cheers,
Claus
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#24 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AYS8sMbMQKBOZ2Iq71wh_nEFmtXq-tkHks5rYhOJgaJpZM4LzmFf>.
|
...new attempt of attaching a patch... |
...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 :-) |
Are you able to also share a dataset for testing? |
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. |
@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. |
@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 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. |
Ok, I spent more time and managed to reproduce the bug. In the test dataset When exporting this building to COLLADA, some polygons of the 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. |
Ah! Great! And this teaches us all (also me) that my data is not necessarily similar to your data. :-) |
@runaas @clausnagel @FxKu , Many thanks for testing and confirming the bug. |
@runaas Yeah, that's why test datasets are always important for us. |
This should be fixed by now as parts of vcDB have been merged with master. Correct @clausnagel ? |
Yes. It should have been solved for the current version as well, but obviously the issue was never closed. So I close it now. |
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...
The text was updated successfully, but these errors were encountered: