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

Added fixed dae models compatible with OpenWebGL #105

Merged
merged 4 commits into from Feb 20, 2019

Conversation

RDaneelOlivav
Copy link
Contributor

Here we added new versions of the Dae files that are compatible with OpenWebGL and minor material tweeks like the red emergency button.

@erelson
Copy link
Collaborator

erelson commented Feb 18, 2019

Could you provide some summary information on what changes are needed to ensure openwebgl compatibility? I would like to pass that info on to the person who historically creates these files.

Additionally, assuming you intend this to be merged eventually, go ahead and give the files the actual names (no .fix) 👍

Thanks!

@RDaneelOlivav
Copy link
Contributor Author

@erelson Just committed the name changes to their original names. Please let me know if you fetchers accepts and merge the changes to the main repo so that I can update everywhere where we are using these files to use the main repo.

For future changes, I suppose this fork and pull request method is the one to use?

As for the method to make dae files compatible with OpenWebGL, its quite tricky and I can't tell you exactly what has to be done with precision. The problem is that WebGL has some issues with certain ways of exporting textures and materials and depending on the program used and method to export those files, they just don't appear or they appear without textures. I recommend using plain textures mapped with UV or materials using Blender render ( not cycles material systems ). Sorry, I can't give you more details, but that's the way we generated these dae files, iteratively until they worked in both Gazebo and WebGL. If someone knows more about the topic please let us know.

@@ -565,7 +565,7 @@
<child link="laser_link" />
<axis xyz="0 0 0" />
</joint>
<link name="torso_fixed_link">
<link name="torsoed_link">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep the original link/joint name of torso_fixed_joint, as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok Fixed I think now. Made a replace to fast ;).
Tell me if there is anything else and notify me if we merge

@erelson
Copy link
Collaborator

erelson commented Feb 19, 2019

Thanks for the quick fixes! I've gotta check/test with a few others before going ahead and merging this.

If you're able to provide screenshots in RViz before/after, that will help demonstrate that the changes are tested and look OK.

@RDaneelOlivav
Copy link
Contributor Author

screenshot from 2019-02-19 20-56-31
screenshot from 2019-02-19 21-02-42
screenshot from 2019-02-19 21-03-21

Is that ok? I dont have the footage that showed how it appered before, didnt print it.

@mfbailey91
Copy link

LGTM

@@ -587,7 +587,7 @@
</geometry>
</collision>
</link>
<joint name="torso_fixed_joint" type="fixed">
<joint name="torsoed_joint" type="fixed">
Copy link
Collaborator

Choose a reason for hiding this comment

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

@RDaneelOlivav sorry I didn't specify before, but there were multiple instances needing to be reverted to the original.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I had fixed that in this commit: 47b9745

Is there something I'm missing here? There were two places where there was an error. Is there anywhere else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this is weird. Your commit's (the most recent one) diffs look correct, but when viewing the full diffs, the current version of the file is wrong for this line. I can easily fix that and am doing so now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There were actually three places, I see now. 👍

@erelson erelson merged commit 20cd76f into ZebraDevs:melodic-devel Feb 20, 2019
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.

None yet

3 participants