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

Add test to ensure binary STL files from SOLIDWORKS get imported correctly #917

Merged
merged 1 commit into from
Nov 4, 2022
Merged

Add test to ensure binary STL files from SOLIDWORKS get imported correctly #917

merged 1 commit into from
Nov 4, 2022

Conversation

ijnek
Copy link
Contributor

@ijnek ijnek commented Oct 30, 2022 •
edited
Loading

This fixes a commonly faced issue with binary STL files from SOLIDWORKS, where they start with the word "solid", which suggests that its an ascii stl file, but really its a binary stl file.

This PR works around the problem by looking for "endsolid" too, as the files from SOLIDWORKS seem to not contain "endsolid".

Resolves: #888

Signed-off-by: Kenji Brameld kenjibrameld@gmail.com

@ijnek
add test to ensure binary STL files from SOLIDWORKS get imported with…
b5083e1 
…out a warning

Signed-off-by: Kenji Brameld <kenjibrameld@gmail.com>
clalancette
clalancette reviewed Oct 31, 2022
View reviewed changes
rviz_rendering/src/rviz_rendering/mesh_loader_helpers/stl_loader.cpp
"a binary STL file. Trying to interpret it as a binary "
"STL file instead.");
if (buffer_str.substr(0, 5) == std::string("solid") &&
buffer_str.find("endsolid", 5) != std::string::npos)
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't say I love this; it means that for every solidworks STL file that is binary, we have to spend the time searching through the whole thing just to determine that it is not ASCII and doesn't contain endsolid. That could be slow, especially on large STL files.

Is there anything more efficient we can do here? The specification for STL is at http://www.fabbers.com/tech/STL_Format , maybe there are some clues on how we can do this more efficiently.

Copy link
Contributor Author

@ijnek ijnek Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't changed the search method in this PR, the efficiency is exactly the same. The only thing that has changed is that now STL files starting with a "solid" but without an "endsolid", get through the checks without a warning.

I agree with the existing code being inefficient, but I suggest that should be addressed in another PR or tracked in an issue for the time being.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair, can you please open an issue about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - #919

@ijnek ijnek mentioned this pull request Nov 3, 2022
Open
clalancette
clalancette approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me. Will run CI next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ijnek
Copy link
Contributor Author

ijnek commented Nov 3, 2022

Had a quick look at CI output, but I can't tell where the failure is coming from.

@clalancette
Copy link
Contributor

It's a CMake warning from Fast-DDS, which should be solved now.

With that, this looks good so I'll go ahead and merge it. Thanks for the contribution!

@clalancette clalancette merged commit 8edb674 into ros2:rolling Nov 4, 2022
@ijnek ijnek deleted the ijnek-solidworks-binary-stl branch November 4, 2022 23:06
@ijnek
Copy link
Contributor Author

ijnek commented Nov 4, 2022

Thanks @clalancette ! May I ask for a backport of this? I believe this is a backport-worthy bug fix that removes unnecessary warnings for anyone using SOLIDWORKS stl files in older distros.

@ijnek
Copy link
Contributor Author

ijnek commented Nov 17, 2022

Ping @clalancette

@ijnek
Copy link
Contributor Author

ijnek commented Dec 3, 2022

Another gentle ping @clalancette

@ahcorde
Copy link
Contributor

ahcorde commented Dec 5, 2022

https://github.com/Mergifyio backport humble galactic foxy

@mergify mergify bot mentioned this pull request Dec 5, 2022
Merged
mergify bot pushed a commit that referenced this pull request Dec 5, 2022
@ijnek @mergify
add test to ensure binary STL files from SOLIDWORKS get imported with…
db9d3a6 
…out a warning (#917)

Signed-off-by: Kenji Brameld <kenjibrameld@gmail.com>
(cherry picked from commit 8edb674)
@mergify mergify bot mentioned this pull request Dec 5, 2022
Merged
mergify bot pushed a commit that referenced this pull request Dec 5, 2022
@ijnek @mergify
add test to ensure binary STL files from SOLIDWORKS get imported with…
af2536b 
…out a warning (#917)

Signed-off-by: Kenji Brameld <kenjibrameld@gmail.com>
(cherry picked from commit 8edb674)
mergify bot pushed a commit that referenced this pull request Dec 5, 2022
@ijnek @mergify
add test to ensure binary STL files from SOLIDWORKS get imported with…
17eafc0 
…out a warning (#917)

Signed-off-by: Kenji Brameld <kenjibrameld@gmail.com>
(cherry picked from commit 8edb674)
@mergify mergify bot mentioned this pull request Dec 5, 2022
Merged
@mergify
Copy link

mergify bot commented Dec 5, 2022

backport humble galactic foxy

✅ Backports have been created

@ijnek
Copy link
Contributor Author

ijnek commented Dec 5, 2022

Thanks @ahcorde !

ahcorde pushed a commit that referenced this pull request Dec 5, 2022
@mergify
add test to ensure binary STL files from SOLIDWORKS get imported with…
ef416f7 
…out a warning (#917) (#933)

Signed-off-by: Kenji Brameld <kenjibrameld@gmail.com>
(cherry picked from commit 8edb674)
ahcorde pushed a commit that referenced this pull request Dec 5, 2022
@mergify
add test to ensure binary STL files from SOLIDWORKS get imported with…
b45471c 
…out a warning (#917) (#931)

Signed-off-by: Kenji Brameld <kenjibrameld@gmail.com>
(cherry picked from commit 8edb674)
ahcorde pushed a commit that referenced this pull request Dec 5, 2022
@mergify
add test to ensure binary STL files from SOLIDWORKS get imported with…
cb1cc96 
…out a warning (#917) (#932)

Signed-off-by: Kenji Brameld <kenjibrameld@gmail.com>
(cherry picked from commit 8edb674)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

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

Successfully merging this pull request may close these issues.

rviz2 complains about a binary stl file with a 'solid' word, but loads.
3 participants
@ijnek @clalancette @ahcorde