-
Notifications
You must be signed in to change notification settings - Fork 15
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
Zero integer values lost on serialization #16
Comments
Hi, can you provide a (minimal) reproducible case? Thanks. |
Sure, I can provide real-world case (it is not minimal, but i want to use untouched tile, as generated by mapnik-vector-tile, to prevent any errors on my side). See tile attached. Now just deserialize it into stream, serialize it back (save it to file maybe, i do that), deserialize this new file, and the same |
started a unit test to reproduce this issue, see https://github.com/bertt/mapbox-vector-tile-cs/blob/master/tests/mapbox.vector.tile.tests/DeserializeSerializeBackTests.cs if you can add some missing code it will be great, otherwise I'll complete it later this week. |
Thanks I've added the code from 067c508#commitcomment-27925502 unit test is indeed failing, let's find a way to fix it |
so this only happens when attribute type=int and value=0? or also with other types? if so, then it should be related to the default values, for example https://github.com/bertt/mapbox-vector-tile-cs/blob/master/src/VectorTile.cs#L68 |
unit test is working when changing https://github.com/bertt/mapbox-vector-tile-cs/blob/master/src/VectorTile.cs#L68 from '[System.ComponentModel.DefaultValue(default(long))]' to '[System.ComponentModel.DefaultValue(int.MinValue)]' lets determine impact for other types. |
my guess is that is happens when the value is default for the particular type (such as 0 for Int32). Since empty string is not default for string, and nulls are not supported by MVT specification, I suspect no problem here. But of course, needs to be tested. |
i've created a new NuGet package (4.0.1) can you try (once it's indexed)? |
@bertt I dont use nuget package, I have sources cloned in my project, since I extended them slightly (for example implemented Add to the unit test:
|
ah ok, little bit too quick :-) I'll take a look later this week again |
sure, thanks, I will try to look into it too, but I am pretty swamped. Btw, I recommend you remove nuget 4.0.1. since it can cause regressions even for people not suffering the issue we are solving. |
yes good idea, 4.0.1 is unlisted |
FYI, since I think this has basically nothing to do with mapbox-vector-tile-cs and it is protobuf-net issue, I raised the question there: protobuf-net/protobuf-net#363 |
ok great! I've reverted VectorTile.cs so the back-to-back serialize unit test fails again. Also added the checks on HasFloatValue and HasStringValue |
So, guys from protobuf-net proposed easy solution for this, see protobuf-net/protobuf-net#363 (comment) You just need to add this code into
I have tested in on my local version and my use case, and it works. |
great it works, all unit tests working again. Closing this issue. |
Hi, first thing first - great project, huge timesaver. I know it is intended only as decoder, but I am using it with success to do minor edits to vector tiles - deserialize, rename some layers, remove some layers, remove some features from layers, and serialize back. It generally works great, with one exception - attributes (properties) which are for example integer value zero, and are correctly deserialized as
HasIntValue=true
andIntValue=0
are not correctly serialized back (if you deserialize the tile serialized this way,HasIntValue' is
falsein that
Value` object.Do you think this can be fixed easily somehow? Thanks!
The text was updated successfully, but these errors were encountered: