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.

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

Jump to bottom

URDF Exporter Gem #5

Merged
merged 25 commits into from
Feb 17, 2023
Merged

URDF Exporter Gem #5

merged 25 commits into from
Feb 17, 2023

Conversation

shawstar
Copy link
Contributor

@shawstar shawstar commented Feb 7, 2023

Please read ReadME, before using gem, please read instructions in full.
Signed-off-by: shawstar shawstar@amazon.com

@shawstar
adding first pr upload @shawstar
9140d01 
Signed-off-by: shawstar <shawstar@amazon.com>
@hultonha hultonha requested review from HogJonny-AMZN, benblack75 and a team February 8, 2023 10:45
@hultonha
Copy link

hultonha commented Feb 8, 2023

@shawstar Would it be possible to include some screenshots of the tool in the PR please to make it a little clearer to see what's being added. Thanks!

@hultonha
Copy link

hultonha commented Feb 8, 2023

Adding some Python experts to review the Python code too

hultonha
hultonha reviewed Feb 8, 2023
View reviewed changes
Copy link

@hultonha hultonha left a comment

Choose a reason for hiding this comment

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

Leaving a preliminary review, I haven't been through all the Python code or README yet but I will be back before too long I promise. This is looking like an awesome start though, great work @shawstar! 👍

Gems/O3DE/urdf-exporter-gem/Code/Include/urdf-exporter-gem/urdf-exporter-gemBus.h Outdated Show resolved Hide resolved
Gems/O3DE/urdf-exporter-gem/Code/Source/Tools/urdf-exporter-gemEditorModule.cpp Outdated Show resolved Hide resolved
Gems/O3DE/urdf-exporter-gem/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/urdf-exporter-gem/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/urdf-exporter-gem/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/urdf-exporter-gem/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/urdf-exporter-gem/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/urdf-exporter-gem/README.md Outdated Show resolved Hide resolved
@shawstar
Copy link
Contributor Author

shawstar commented Feb 8, 2023

@shawstar Would it be possible to include some screenshots of the tool in the PR please to make it a little clearer to see what's being added. Thanks!

Yeah, the github gem readme has them all: https://github.com/o3de/o3de-technicalart/tree/shawstar/urdf-exporter-gem/Gems/O3DE/urdf-exporter-gem

@shawstar
Copy link
Contributor Author

shawstar commented Feb 8, 2023

Leaving a preliminary review, I haven't been through all the Python code or README yet but I will be back before too long I promise. This is looking like an awesome start though, great work @shawstar! +1

Thanks Tom that means allot!

@shawstar
PR editor_tool print fixs
5547994 
Signed-off-by: Starr Shaw <shawstar@amazon.com>
FuzzyCarter
FuzzyCarter requested changes Feb 16, 2023
View reviewed changes
Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/utilities.py Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/utilities.py Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/utilities.py Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/utilities.py Outdated Show resolved Hide resolved
Gems/O3DE/UrdfExporter/Editor/Scripts/utilities.py Show resolved Hide resolved
@cgalvan cgalvan mentioned this pull request Feb 16, 2023
Open
shawstar and others added 2 commits February 16, 2023 14:06
@shawstar @michalpelka
Update Gems/O3DE/UrdfExporter/README.md
f7aebe2 
Co-authored-by: Michał Pełka <michalpelka@gmail.com>
Signed-off-by: Starr Shaw <87207603+shawstar@users.noreply.github.com>
@shawstar @michalpelka
Update Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py
f6f17e2 
Co-authored-by: Michał Pełka <michalpelka@gmail.com>
Signed-off-by: Starr Shaw <87207603+shawstar@users.noreply.github.com>
@shawstar
Copy link
Contributor Author

This looks like a great feature, nice work! Couple bits of feedback

* Normally we don't use the word "Gem" in our gem titles because it makes it a bit awkward to say in a sentence  "Just enable the UrdfExporterGem gem in your project", and we also don't use dashes in the gem name or folders.  I recommend renaming it from `urdf-exporter-gem` to `URDFExporter` or `UrdfExporter`

* As mentioned in other reviews unlike Python, our c++ coding standards use CamelCase for all class names, function names, service names etc.

Should be all good now.

@shawstar
Copy link
Contributor Author

@shawstar shawstar closed this Feb 16, 2023
@shawstar
Copy link
Contributor Author

A lot of good work here! I found some issues that should be rectified.
I also tried to follow the tutorial in the readme and failed, I do not know why exactly.
I also think that these tools should be compatible with the URDF importer.
In other words, the exporter should produce a valid URDF from a prefab created with a URDF importer from ROS2Gem.

I have exported and imported into the URDF Robotech.ai importer. Do you know which step of the the tutorial you had issues?

@shawstar shawstar reopened this Feb 16, 2023
shawstar and others added 5 commits February 17, 2023 09:51
@shawstar
Fri Morn PR fix @shawstar
c91d716 
Signed-off-by: Starr Shaw <shawstar@amazon.com>
@shawstar
PR changes friday mid day @shawstarr
8c3f759 
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar
PR Changes from Linux @shawstar
f27a2dc 
Signed-off-by: Starr Shaw <shawstar@amazon.com>
@shawstar
Merge branch 'shawstar/urdf-exporter-gem' of https://github.com/o3de/…
d2e602a 
…o3de-technicalart into shawstar/urdf-exporter-gem

PR from Linux
@shawstar
Update README.md
44a3ee8 
Signed-off-by: Starr Shaw <87207603+shawstar@users.noreply.github.com>
FuzzyCarter
FuzzyCarter approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@FuzzyCarter FuzzyCarter left a comment

Choose a reason for hiding this comment

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

This is looking great.

Have some minor feedback on a function that looks like it's not doing what you intended.

Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py Outdated Show resolved Hide resolved
@shawstar @FuzzyCarter
Update Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py
5a47a82 
Co-authored-by: Fuzzy Carter <jscar@amazon.com>
Signed-off-by: Starr Shaw <87207603+shawstar@users.noreply.github.com>
Signed-off-by: shawstar <shawstar@amazon.com>
shawstar and others added 9 commits February 17, 2023 14:14
@shawstar
Fix for a type hint Any @shawstar
65c479b 
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar
Fix for a type hint Any @shawstar
25c1dbb 
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar
Merge branch 'shawstar/urdf-exporter-gem' of https://github.com/o3de/…
9ecb25a 
…o3de-technicalart into shawstar/urdf-exporter-gem
@shawstar
Fri Morn PR fix @shawstar
d354533 
Signed-off-by: Starr Shaw <shawstar@amazon.com>
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar
PR Changes from Linux @shawstar
a3bc24f 
Signed-off-by: Starr Shaw <shawstar@amazon.com>
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar
Update README.md
fc10617 
Signed-off-by: Starr Shaw <87207603+shawstar@users.noreply.github.com>
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar @FuzzyCarter
Update Gems/O3DE/UrdfExporter/Editor/Scripts/editor_tools.py
c348caf 
Co-authored-by: Fuzzy Carter <jscar@amazon.com>
Signed-off-by: Starr Shaw <87207603+shawstar@users.noreply.github.com>
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar
Fix for a type hint Any @shawstar
ec4bac4 
Signed-off-by: shawstar <shawstar@amazon.com>
@shawstar
Merge branch 'shawstar/urdf-exporter-gem' of https://github.com/o3de/…
b1b6189 
…o3de-technicalart into shawstar/urdf-exporter-gem
@HogJonny-AMZN HogJonny-AMZN merged commit 61ed9c4 into development Feb 17, 2023
@shawstar shawstar deleted the shawstar/urdf-exporter-gem branch February 17, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalpelka michalpelka michalpelka left review comments

@jckand jckand jckand left review comments

@hultonha hultonha hultonha approved these changes

@FuzzyCarter FuzzyCarter FuzzyCarter approved these changes

@HogJonny-AMZN HogJonny-AMZN Awaiting requested review from HogJonny-AMZN

@benblack75 benblack75 Awaiting requested review from benblack75

@Kadino Kadino Awaiting requested review from Kadino

@AMZN-alexpete AMZN-alexpete Awaiting requested review from AMZN-alexpete

@amzn-ahmadkrm amzn-ahmadkrm Awaiting requested review from amzn-ahmadkrm

@spham-amzn spham-amzn Awaiting requested review from spham-amzn

@pawelbudziszewski pawelbudziszewski Awaiting requested review from pawelbudziszewski

@adamdbrw adamdbrw Awaiting requested review from adamdbrw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@shawstar @hultonha @AMZN-alexpete @michalpelka @FuzzyCarter @jckand @spham-amzn @amzn-ahmadkrm @HogJonny-AMZN