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

Invisible shape identification #60

Merged
merged 24 commits into from
Jun 20, 2024
Merged

Conversation

kklemon
Copy link
Collaborator

@kklemon kklemon commented Jun 6, 2024

Closes #52.

While this was initially meant as a simple-to-implement improvement for the bounding box generation for the VLMs, it turned out to be much more difficult to implement and particularly to test than anticipated.

The few simple rules that were sketched in #52 were not sufficient and consideration of many different edge cases and interactions between various SVG element attributes was necessary.

In the end, I tried to keep the logic as simple as possible with the aim to reduce the false positive rate while possibly missing a few invisible shapes. The provided tests do in fact test for pixel perfectness between the original and cleaned version and and thus should guarantee a FPR of 0, given that the tests are correct.

Here is the effect visualized with bounding boxes:

frame-with-visible-shapes frame-invisible-shapes-removed

TODO:

  • Systematically evaluate with more cases (no hard requirement for merge)

src/penai/svg.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@MischaPanch MischaPanch left a comment

Choose a reason for hiding this comment

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

Overall looks good, just minor comments

src/penai/svg.py Outdated
@@ -538,6 +566,20 @@ def is_container_type(self) -> bool:
def is_primitive_type(self) -> bool:
return self._shape_type.value.category == PenpotShapeTypeCategory.PRIMITIVE

@property
def produces_visible_content(self) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we turn this into a method pls? I expect properties access to be essentially instantaneous (like attr access)

src/penai/svg.py Outdated Show resolved Hide resolved
src/penai/svg.py Outdated Show resolved Hide resolved
src/penai/svg.py Outdated Show resolved Hide resolved
src/penai/svg.py Show resolved Hide resolved
@kklemon
Copy link
Collaborator Author

kklemon commented Jun 20, 2024

@MischaPanch After no progress last week, I have now updated the code and fixed the outstanding issues. You can merge it if you are happy with the current state.

src/penai/xml.py Outdated
if key.startswith("ns"):
from lxml import etree

print(etree.tostring(self))
Copy link
Collaborator

@MischaPanch MischaPanch Jun 20, 2024

Choose a reason for hiding this comment

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

pls no printing in properties (or in fact almost anywhere). Did this whole block sneak in accidentally during debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should stop doing debug sessions after midnight...

@MischaPanch
Copy link
Collaborator

MischaPanch commented Jun 20, 2024

@kklemon LGTM! Just minor comments about something that looks like forgotten/debugging code. Feel free to merge yourself after fixing

hook,
):
# Even though the underlying SVGs have the perfectly same visible content, they still
# might produc
Copy link
Collaborator

Choose a reason for hiding this comment

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

finish sentence or remove ;)

@kklemon kklemon merged commit a2c73ea into main Jun 20, 2024
2 checks passed
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.

Remove shapes corresponding to no visible content
2 participants