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

Add example vehicle #112

Conversation

ClemensLinnhoff
Copy link
Collaborator

@ClemensLinnhoff ClemensLinnhoff commented Sep 19, 2024

Describe your changes

Add example vehicle in glTF format to showcase the definitions of the vehicle node structure.

  • Fix structure according to new definitions
  • Fix huge bevels at doors (all edges) and the convertible top (front/tail edge)

Issue ticket number and link

Fixes #111

Checklist before requesting a review

  • I have performed a self-review of my code/documentation.
  • My changes generate no new warnings during the documentation generation.

Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff ClemensLinnhoff linked an issue Sep 19, 2024 that may be closed by this pull request
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff
Copy link
Collaborator Author

I updated the 3D model to the current definition of the vehicle structure. Please review the internal structure, node naming, coordinate origins (pivot points) etc.
I also added a corresponding asset file (without material mapping).

Copy link
Member

@AsamDiegoSanchez AsamDiegoSanchez left a comment

Choose a reason for hiding this comment

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

General check, looks ok to me.
Signed-off-by: Diego Sánchez [email protected]

@LudwigFriedmann
Copy link
Collaborator

LudwigFriedmann commented Oct 29, 2024

Remarks:

  • [Example issue]
    Keyword for vehicle part is missing (not yet defined)

    • ""Solution: Vehicle part and corresponding keyword concept is postponed for now""
  • [Example issue]
    Lateral iterators are inconsistent for low beam lights (0 = left) and wheels (0=right).
    This may be true for other nodes, as well.

  • [Concept Issue]
    Side mirrors and mountings are not affected by door animation.
    They could be either mounted to the fenders or they have to become children to the doors.

    • Solution: Add Side_Mirror_Left Mountings_to Nodes Grp_Dynamic_Exterior and Grp_Door Left as childs
      • Solution: Add Side_Mirror_Right_Mountings to Nodes Grp_Dynamic_Exterior and Grp_Door Right as childs
      • Add corresponding Mirror_Views to these Nodes as Childs
      • Use consistend indices for mirrors mounted to car body or doors to indicate the lateral positioning of the corresponding mirrors
  • [Concept Issue]
    Coordinate frames of the side mirrors are positioned in the mirror mounting joints, not in the center of the mirror surface.
    This is helpful for animation of mirror adjustment, but a coordinate for reflections or mirror cameras is missing.
    A solution could be an additional child coordinate frame in the center of the mirror surface, aligned with the tangens/normal of the mirror.

    • Solution: Add another Coordinate Frame Mirror_Joint so that the hierarchy resembles Mounting (Coordinate origin in contact surface with body/door)->Joint (Coordinate origin in joint)->View (Coordinate origin in center of mirror glass surface, aligned with tangents/normals or mirror glas, empty node)
  • [Concept Issue]
    Same is true for the rearview mirror. If the mirror hinge doesn't coincide with the center of the mirror surface, we need an additional coordinate frame to position cameras or calculate reflections.

    • Solution: see above
  • [Concept Issue]
    Due to the vehicle part indices, the structure is no longer easy to read.

    • Proposal: Remove vehicle part concept for now. State notes that in case a user implements vehicle parts or similar, he may follow rules like indexing that are use in the standardized structure. The bounding box that is used to calculate the vehicle reference coordinate frame will always include the full vehicle.

    image

  • [Artistic issue]
    Huge bevels at doors (all edges) and the convertible top (front/tail edge) should be removed.
    (in Blender select edges in Edit Mode and apply Edge -> Mark Sharp; add Edge Crease if required)

@ClemensLinnhoff
Copy link
Collaborator Author

Thanks @LudwigFriedmann, let's discuss your remarks in the meeting tomorrow (especially the conceptual issues).

@ClemensLinnhoff
Copy link
Collaborator Author

  • [Example issue]
    Lateral iterators are inconsistent for low beam lights (0 = left) and wheels (0=right).
    This may be true for other nodes, as well.

I checked this again and I think it is correct in the example. We only have one Light_Low_Beam_Left. So the index 0 is correct in my opinion.
The index for the wheels also is correct, as we count in positive y-direction from right to left.

Do you agree @LudwigFriedmann ?

@LudwigFriedmann
Copy link
Collaborator

LudwigFriedmann commented Nov 7, 2024

@ClemensLinnhoff : You're correct, I missed the "Left" in Light_Low_Beam_Left and misinterpreted the 0 as a lateral iterator.
In contrast, wheels feature a lateral iterator and no "left" or "right".
Should we harmonize that?

@ClemensLinnhoff
Copy link
Collaborator Author

@ClemensLinnhoff : You're correct, I missed the "Left" in Light_Low_Beam_Left and misinterpreted the 0 as a lateral iterator. In contrast, wheels feature a lateral iterator and no "left" or "right". Should we harmonize that?

The wheels are harmonized with OSI, so I would not change their definition.
For the lights I think we defined them as left and right because they are typically on either side of the vehicle. So in my opinion it would be fine to stick with the current definition.

@ClemensLinnhoff
Copy link
Collaborator Author

I updated the example vehicle according to the review remarks and the changes to the node structure introduced in #171.
@LudwigFriedmann please re-review.

@LudwigFriedmann
Copy link
Collaborator

LudwigFriedmann commented Nov 15, 2024

Hi @ClemensLinnhoff, thanks for all the improvements.

I have a few minor remarks, only:

[Concept issue]
We should have a note somewhere that only node names are subject to standardization.
For meshes, arbitrary names can be chosen.

[Concept/Example issue]
For doors and lights, we use suffixes "Left" and "Right".
For Wheels, we use longitudinal and lateral iterators.
Seats introduce a new, 2-dimensional iterator concept.
At least the seat iterarators should be harmonized.

[Artistic issue]
There are still some rendering problems at

  • front and back edges of the convertible top
  • inner edges of the trunk lid
  • edges around the rear lights

This happens because "Shade smooth" is enabled for the smooth parts.
As Blender doesn't know which edges are meant to stay sharp, it tries to smoothen all edges by introducing bevels.
For sharp edges and contact surfaces/edges, specifying "Sharp edges" should solve the problem.

Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff
Copy link
Collaborator Author

We fixed the artistic issue. But we had to do a complete remesh, because there were too many imperfections from the original version. Now it should be fine.

@LudwigFriedmann
Copy link
Collaborator

Decision: Grp_Sensor_<sensor_idx> should be renamed to Grp_Rear_Axle_Center and the index should be removed (it's an empty node)

@ClemensLinnhoff
Copy link
Collaborator Author

I added the rear axle center to the definition as well as the glTF.

Now just the seat index is open. @LudwigFriedmann I don't remember if we made a final decision about this in the meeting.

@LudwigFriedmann
Copy link
Collaborator

Hi @ClemensLinnhoff, as far as I remember, the most favored solution was to have a row index and a row-specific seat index.

Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff
Copy link
Collaborator Author

Hi @ClemensLinnhoff, as far as I remember, the most favored solution was to have a row index and a row-specific seat index.

Okay thanks. I fixed the naming both in the definitions and in the glTF.

Copy link
Collaborator

@LudwigFriedmann LudwigFriedmann left a comment

Choose a reason for hiding this comment

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

The car looks great and the documentation reflects this. Thanks for all the work!

@LudwigFriedmann LudwigFriedmann merged commit ca0dd6e into main Nov 21, 2024
2 checks passed
@ClemensLinnhoff ClemensLinnhoff deleted the 111-add-example-vehicle-to-demonstrate-vehicle-node-structure branch December 18, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add example vehicle to demonstrate vehicle node structure
3 participants