Skip to content

Fix flatbuffer builds for upcoming release #730

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 21, 2023
Merged

Fix flatbuffer builds for upcoming release #730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8b399b
Fix flatbuffer build against previous build changes
pmai Jun 15, 2023
55bfcf7
Switch to findpackage for flatbuffers dependency
pmai Jun 15, 2023
1f113a3
Reorder enum declaration and use for duplicated type names
pmai Jun 15, 2023
ba5b394
Add missing project-scope alias for flatbuffers library
pmai Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .gitmodules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +0,0 @@
[submodule "flatbuffers"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit surprised that the submodule is deleted here and find_package is used in CMakeLists instead. @pmai why do you suggest this particular change?

Background info: I find it very convenient to use a submodule here and not have the dependency on an (possibly) existing package. It is one of the pro-points on the list for flatbuffers that this is somehow possible..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I also found this somewhat convenient, it has its own problems (both in nailing down the version of flatbuffers needlessly, and not making it easy to configure for different target settings), and it is not what we do for protobuf. And with todays availability of vcpkg and others for installing flatbuffers, it seems better to stay consistently out of the submodule business for these kinds of dependencies.

Copy link
Contributor

@PhRosenberger PhRosenberger Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. On the other side, nailing down the version of flatbuffers (and possibly protobuf) could prevent some missunderstandings in combining different models / tool plugins. The flexibility to configure for different target settings is indeed very important for our project. So if using submodules can be a problem to this point, I am ok with the change here.

path = flatbuffers
url = https://github.com/google/flatbuffers.git
20 changes: 11 additions & 9 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,10 @@ protobuf_generate_cpp(PROTO_SRCS PROTO_HEADERS ${OSI_PROTO_FILES})
set(FLAT_HEADERS "")
if(OSI_BUILD_FLATBUFFER)
set(FLAT_FBS "")
add_subdirectory("flatbuffers"
${CMAKE_CURRENT_BINARY_DIR}/flatbuffers-build
EXCLUDE_FROM_ALL)
find_package(flatbuffers REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above.

if(NOT FLATBUFFERS_FLATC_EXECUTABLE)
set(FLATBUFFERS_FLATC_EXECUTABLE ${flatbuffers_DIR}/../../tools/flatbuffers/flatc)
endif()
file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/descriptor.fbs" "namespace osi3;")
file(MAKE_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/include")
list(APPEND FLAT_FBS "${CMAKE_CURRENT_BINARY_DIR}/descriptor.fbs")
Expand All @@ -111,9 +112,9 @@ if(OSI_BUILD_FLATBUFFER)
set(fbs "${proto_base}.fbs")
add_custom_command(
OUTPUT "${fbs}"
COMMAND $<TARGET_FILE:flatc> -I "${PROTOBUF_IMPORT_DIRS}" -o "${CMAKE_CURRENT_BINARY_DIR}" --proto "${CMAKE_CURRENT_SOURCE_DIR}/${proto}"
DEPENDS "${proto}" flatc
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
COMMAND ${FLATBUFFERS_FLATC_EXECUTABLE} -I "${PROTOBUF_IMPORT_DIRS}" -I "${CMAKE_CURRENT_BINARY_DIR}" -o "${CMAKE_CURRENT_BINARY_DIR}" --proto "${proto}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change fixing an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes:

  • Use of -I due to the cleaner separation of SOURCE and BINARY directories (we formerly generated files in the SOURCE directory, which we shouldn't)
  • Since flatbuffers are not longer co-built, the flatc executable is not a target anymore, but rather referenced via variable.

DEPENDS "${proto}"
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
COMMENT "Convert ${proto} to ${fbs} using flatc"
)
list(APPEND FLAT_FBS "${CMAKE_CURRENT_BINARY_DIR}/${fbs}")
Expand All @@ -125,8 +126,8 @@ if(OSI_BUILD_FLATBUFFER)
set(fbh "${flat_base}_generated.h")
add_custom_command(
OUTPUT "include/${fbh}"
COMMAND $<TARGET_FILE:flatc> -o "${CMAKE_CURRENT_BINARY_DIR}/include" --cpp --gen-mutable --gen-name-strings --scoped-enums "${fbs}"
DEPENDS "${FLAT_FBS}" flatc
COMMAND ${FLATBUFFERS_FLATC_EXECUTABLE} -o "${CMAKE_CURRENT_BINARY_DIR}/include" --cpp --gen-mutable --gen-name-strings --scoped-enums "${fbs}"
DEPENDS "${FLAT_FBS}"
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
COMMENT "Process ${fbs} to ${fbh} using flatc"
)
Expand All @@ -135,9 +136,10 @@ if(OSI_BUILD_FLATBUFFER)

add_custom_target(${PROJECT_NAME}_fbs_build ALL DEPENDS "${FLAT_HEADERS}")
add_library(${PROJECT_NAME}_fbs INTERFACE)
add_library(${PROJECT_NAME}::${PROJECT_NAME}_fbs ALIAS ${PROJECT_NAME}_fbs)
target_include_directories(${PROJECT_NAME}_fbs INTERFACE $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include/>)
target_include_directories(${PROJECT_NAME}_fbs SYSTEM INTERFACE $<INSTALL_INTERFACE:$<INSTALL_PREFIX>/include>)
target_link_libraries(${PROJECT_NAME}_fbs INTERFACE flatbuffers)
target_link_libraries(${PROJECT_NAME}_fbs INTERFACE flatbuffers::flatbuffers)
endif()

add_library(${PROJECT_NAME}_static STATIC ${PROTO_SRCS} ${PROTO_HEADERS})
Expand Down
1 change: 0 additions & 1 deletion flatbuffers
Open in desktop
Submodule flatbuffers deleted from 6df40a
43 changes: 22 additions & 21 deletions osi_lane.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,27 +973,6 @@ message LaneBoundary
//
message Classification
{
// The type of the lane boundary.
//
optional Type type = 1;

// The semantic color of the lane boundary in case of lane markings.
//
// \note The color types represent the semantic classification of
// lane markings only. They do not represent an actual visual appearance.
//
optional Color color = 2;

// The ids of \c StationaryObject which limit the corresponding lane.
// This field must be set if the \c #type is set to
// \c #TYPE_STRUCTURE
//
// \rules
// refers_to: StationaryObject
// \endrules
//
repeated Identifier limiting_structure_id = 3;

// The lane boundary type.
// There is no special representation for double lines, e.g. solid /
// solid or dashed / solid. In such cases, each lane will define its own
Expand Down Expand Up @@ -1070,6 +1049,28 @@ message LaneBoundary
TYPE_SOUND_BARRIER = 15;
}

// The type of the lane boundary.
//
optional Type type = 1;

// The semantic color of the lane boundary in case of lane markings.
//
// \note The color types represent the semantic classification of
// lane markings only. They do not represent an actual visual appearance.
//
optional Color color = 2;

// The ids of \c StationaryObject which limit the corresponding lane.
// This field must be set if the \c #type is set to
// \c #TYPE_STRUCTURE
//
// \rules
// refers_to: StationaryObject
// \endrules
//
repeated Identifier limiting_structure_id = 3;


// The semantic color of the lane boundary in case of a lane markings.
// Lane markings that alternate in color must be represented by
// individual \c LaneBoundary segments.
Expand Down
104 changes: 52 additions & 52 deletions osi_object.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,31 @@ message MovingObject
//
optional BaseMoving base = 2;

// Definition of object types.
//
enum Type
{
// Type of the object is unknown (must not be used in ground truth).
//
TYPE_UNKNOWN = 0;

// Other (unspecified but known) type of moving object.
//
TYPE_OTHER = 1;

// Object is a vehicle.
//
TYPE_VEHICLE = 2;

// Object is a pedestrian.
//
TYPE_PEDESTRIAN = 3;

// Object is an animal.
//
TYPE_ANIMAL = 4;
}

// The type of the object.
//
optional Type type = 3;
Expand Down Expand Up @@ -488,31 +513,6 @@ message MovingObject
//
optional ColorDescription color_description = 11;

// Definition of object types.
//
enum Type
{
// Type of the object is unknown (must not be used in ground truth).
//
TYPE_UNKNOWN = 0;

// Other (unspecified but known) type of moving object.
//
TYPE_OTHER = 1;

// Object is a vehicle.
//
TYPE_VEHICLE = 2;

// Object is a pedestrian.
//
TYPE_PEDESTRIAN = 3;

// Object is an animal.
//
TYPE_ANIMAL = 4;
}

//
// \brief The vehicle attributes for \c MovingObject (host or other).
//
Expand Down Expand Up @@ -727,33 +727,6 @@ message MovingObject
//
message VehicleClassification
{
// The type of the vehicle.
//
optional Type type = 1;

// The light state of the vehicle.
//
optional LightState light_state = 2;

// Flag defining whether the vehicle has an attached trailer.
//
optional bool has_trailer = 3;

// Id of the attached trailer.
//
// \note Field need not be set if has_Trailer is set to false or use
// value for non valid id.
//
// \rules
// check_if this.has_trailer is_equal_to true else do_check is_set
// \endrules
//
optional Identifier trailer_id = 4;

// The role of the vehicle.
//
optional Role role = 5;

// Definition of vehicle types.
//
// \note OSI provides a richer set of vehicle types than is supported by some
Expand Down Expand Up @@ -873,6 +846,33 @@ message MovingObject
TYPE_STANDUP_SCOOTER = 17;
}

// The type of the vehicle.
//
optional Type type = 1;

// The light state of the vehicle.
//
optional LightState light_state = 2;

// Flag defining whether the vehicle has an attached trailer.
//
optional bool has_trailer = 3;

// Id of the attached trailer.
//
// \note Field need not be set if has_Trailer is set to false or use
// value for non valid id.
//
// \rules
// check_if this.has_trailer is_equal_to true else do_check is_set
// \endrules
//
optional Identifier trailer_id = 4;

// The role of the vehicle.
//
optional Role role = 5;

//
// \brief The state of the lights of a vehicle.
//
Expand Down
Loading