Move CityObjectsType & -Mask away from PoT enum values #104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows building the library without the need for a PR. This avoids having to create PRs on forked repositories just to verify the library still builds.
Minimum for GTest is 14 - but I don't see a reason why we shouldn't use C++ 17 by now.
The tests in CTest resp. with the citygmltest executable are now part of the system tests. The source code has been adapted to more modern C++ but taken along.
This prohibits the calling code from compiling methods that weren't used in the main library code. E.g. the copy constructor doesn't get generated. Without this any test ussing the enum type bitmask cannot link correctly.
These tests don't all pass yet - specifically the FilterBuilding tests fail. At this point the tests don't even compile. The next commit will fix this.
Currently this will lead to a stack overflow as the Bitmask operators are implemented by the binary operations on the types. But this prepares for the move away from operations on the underlying type of the CityObjectType enum.
The interface of the two is almost identical and there is no reason to maintain a custom implementation.
Using only PoT values had a hard limit of 64 (or maximum 128) entries in the enum. The CityGML spec has for sure more than 64 CityObject types. Possibly even more than 128.
Without this change existing code like the following will not compile anymore: ParserParams params; params.objectsMask = CityObject::CityObjectsType::COT_All;
This makes sure that legacy code can compile as before again.
This can be seen as a guide on how to transition existing client code to the new interface. Most probably there is no such code because the filtering doesn't work currently.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the
CityObjectsTypeenum is a 64 bit integer and 53 of these bits are already in use with many city object types not yet assigned a number.Having a limit of 64 (or even 128) values is not an option to cover the full CityGML specification. Therefore we need to move to a version that doesn't have such a limit.
This PR:
ParserParams::objectsMaskfiltering which was broken.CityObjectsTypeMaskto use anstd::bitsetbecause its size is unbounded.Since quite some of my PRs aren't merged yet, some are also included in this PR because I find it hard to develop with so many warnings popping up and without tests.