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

docs: fixed texts and links in node diagram #558

Closed
wants to merge 0 commits into from

Conversation

TimS120
Copy link

@TimS120 TimS120 commented May 26, 2024

Description

What I have done:

  • Translated all Japanese texts
  • Checked all links if they are broken and if the reference could be right (and fixed broken links of lateral_controller, longitudinal_controller and accel_map_calibrator)
  • Checked, if all group-blocks are behind the single-blocks (costmap_generator wasn’t reachable (the link behind it, because the group where the node was in it was over it))
  • Background color of car/traffic_light_classifier and pedestrian/traffic_light_classifier fixed

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The Reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Copy link

stale bot commented Jul 25, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Jul 25, 2024
@shmpwk
Copy link
Contributor

shmpwk commented Nov 23, 2024

@TimS120
We are so sorry for not noticing your great PR. Could you check the current conflicts?

@stale stale bot removed the status:stale Inactive or outdated issues. (auto-assigned) label Nov 23, 2024
@TimS120 TimS120 closed this Dec 21, 2024
@TimS120
Copy link
Author

TimS120 commented Dec 21, 2024

@shmpwk is your question still open? Had much to do in the last weeks, but have to make now Christmas-break, so would have now much time...
I spent a short break to identify some points (below) which should also be considered in my opinion when doing the described work in the PR, because that is also related. Please let me know your opinion about it and how to proceed.
[I would see from my perspective some possible proceeding types (since such draw.io-file is hard to oversee regarding the changes):
• You don't have the trust that I do meaningful work and I only check the most significant points, document them and let you/someone know my findings and he/she will do the changes
• You have the trust and I do the described changes with a documentation what exactly I changed (much work)
• You have the trust and I do the described changes

• Something complete different]

Group-boxes over group-elements (components) [Components not accessible for the user without opening the diagram directly in draw.io}:
• LiDAR sensing pipeline
• GNSS sensing pipeline
• …

Components without link [Is no link required, because the component is too trivial? Could still be useful to have a link to (at least) already existing component descriptions]:
• Camera sensing pipeline > camera_driver (both)
• LiDar sensing pipeline > packets_to_pointcloud
• …

Components without description (Tooltip) [To have sometimes additional information but sometimes not is a little bit confusing…]:
• Radar sensing pipeline > Radar_driver
• …

Components with same Tooltip description as name [This gives no additional information, so it is unnecessary to have that (or it is changes to something meaningful)]:
• Camera sensing pipeline > camera_driver
• …

Components with only Japanese Tooltip description [Since everything else is written in English, this should also be written in it (I guess)]:
• LiDAR sensing pipeline > fix_distortion
• …

Questionable colour choice:
• Text of car/traffic_light_classifier and pedestrian/traffic_light_classifier [Why do ONLY they have a white background colour, every other component has no text background colour]

Inconsistent group-box names [Which representation of it should be used? Is it correct to use all of them (because they do represent different things)?]:
• Compare “/perception/occupancy_grid_map/” (Perception) with “system_monitors” (System) with “Lidar sensing pipeline” (Sensing) [What is the difference between them? They all include single components, but have different names (Ros-topic vs. real name) and also different name-conversions (a name with spaces vs. a name with underscores)]

Inconsistent usage of arrows in group elements [Causes inconsistency]:
• Compare “Lidar sensing pipeline” (arrows between each component) vs. “behavior_path_planner” (no connections)

Inconsistent usage of arrows in general [Again causing inconsistency]:
• Arrows have an arch, when they go over a second arrow vs. Arrows do not have an arch, when they go over a second arrow (“gnss_poser” to “pose_initializer” vs. “crosswalk_traffic_light_estimator” to “behaviour_velocity_planner”)

“/api/routing/change_route” in mid air without any relation to anything [Probably an error]

Functionality of “XX1”, “X2”, “X1” and “S1” buttons unclear [Are they maybe outdated?]

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.

2 participants