Skip to content

feat: Logo detection #1873

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

Merged
merged 26 commits into from
Apr 2, 2025
Merged

feat: Logo detection #1873

merged 26 commits into from
Apr 2, 2025

Conversation

smereu
Copy link
Contributor

@smereu smereu commented Mar 27, 2025

Description

Exposes logo detection and removal. Logos is exposed as a problem area defined by a list of faces. There is no ability to distinguish multiple logos if present so we expose a single problem area. The problem area has a fix method to remove the logo. There is a find and remove method that allows to remove everything automatically.

Issue linked

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate unit tests.
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved to the PR if any.
  • I have assigned this PR to myself.
  • I have added the minimum version decorator to any new backend method implemented.
  • I have made sure that the title of my PR follows Conventional commits style (e.g. feat: extrude circle to cylinder)

@smereu smereu requested a review from a team as a code owner March 27, 2025 03:44
@github-actions github-actions bot added the enhancement New features or code improvements label Mar 27, 2025
@smereu smereu changed the title Feat/logo detection 1 Feat: Logo detection Mar 27, 2025
@smereu smereu self-assigned this Mar 27, 2025
@smereu
Copy link
Contributor Author

smereu commented Mar 27, 2025

@RobPasMue My previous branch for logo detection had too many conflicts and I was getting a hard time with it so I created a new one and a new pull request. There is a small fix pending on the API server then it will be ready for full review

@RobPasMue
Copy link
Member

@RobPasMue My previous branch for logo detection had too many conflicts and I was getting a hard time with it so I created a new one and a new pull request. There is a small fix pending on the API server then it will be ready for full review

Hi @smereu - does that mean we can close #1819?

fix string
@smereu smereu changed the title Feat: Logo detection feat: Logo detection Mar 31, 2025
@smereu
Copy link
Contributor Author

smereu commented Mar 31, 2025

@RobPasMue I added some check for disabling on Linux but I am not sure if it is the correct way to do it. Should I log an issue for it? Also there is one change in the API server that has to be merged otherwise one of the asserts will fail.

@RobPasMue
Copy link
Member

@RobPasMue I added some check for disabling on Linux but I am not sure if it is the correct way to do it. Should I log an issue for it? Also there is one change in the API server that has to be merged otherwise one of the asserts will fail.

Hi @smereu -- I can't see any "skip on Linux" condition on your PR, sadly. That's what's causing the CI/CD to fail I guess.

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Requesting a few changes -- nonetheless, the PR is looking good, thanks @smereu!

@smereu
Copy link
Contributor Author

smereu commented Apr 1, 2025

@RobPasMue I hope I addressed the comments. I will add the quantity input to this method together with all the other ones.
The failing test needs a new build of API server. I could address it now but it will start failing with a new build

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Hi @smereu - changes look good. Thanks for addressing them. I think there are only 2 comments missing to be resolved:

1- I'd rather access by name (inside the test) whenever we want to validate number of faces of a given body. I'd search for the component with the desired name - and for the specific body we want (also by name), and then validate its face number.
2- I don't think we need the modeler inside prepare tools based on the operations you are performing (just retrieving the backend type). That information is available inside the client already so there is no need for the modeler =)

smereu and others added 6 commits April 1, 2025 11:25
Removed unnecessary use of modeler and use name instead of index for unit test
Fix typo and set success to expected value once API server is updated
make test pass. To be updated with new build
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 90.19608% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.18%. Comparing base (c202b02) to head (feb9603).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ansys/geometry/core/tools/prepare_tools.py 84.84% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1873      +/-   ##
==========================================
- Coverage   90.18%   90.18%   -0.01%     
==========================================
  Files          92       92              
  Lines        8852     8902      +50     
==========================================
+ Hits         7983     8028      +45     
- Misses        869      874       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Implementation looks good @smereu - thanks for handling all my comments!

@RobPasMue RobPasMue merged commit 8a5567c into main Apr 2, 2025
47 of 48 checks passed
@RobPasMue RobPasMue deleted the feat/logo_detection_1 branch April 2, 2025 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants