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

Skia shapes hittesting #16201

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ramezgerges
Copy link
Contributor

GitHub Issue (If applicable): closes #15855

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@ramezgerges ramezgerges marked this pull request as draft April 10, 2024 12:47
@github-actions github-actions bot added area/skia ✏️ Categorizes an issue or PR as relevant to Skia area/automation Categorizes an issue or PR as relevant to project automation labels Apr 10, 2024
@ramezgerges
Copy link
Contributor Author

this builds on top of #15875

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16201/index.html

Comment on lines +17 to +18
internal SKPath? LastDrawnStrokePath { get; private set; }
internal SKPath? LastDrawnFillPath { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if there is an unmanaged memory leak here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I noticed that too. According to the documentation, the unmanaged memory will be freed in the managed object's destructor.

Copy link
Member

Choose a reason for hiding this comment

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

Especially this they are no longer disposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disposing sometimes throws weird native errors. It should be safe to not dispose. The remark on Dispose says this:

Always dispose the object before you release your last reference to the . Otherwise, the resources it is using will not be freed until the garbage collector calls the finalizer.

image

Copy link
Member

Choose a reason for hiding this comment

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

Not disposing and relying on the GC may have performance impacts, I think. Finalizers aren't cheap, and an explicit Dispose call should be calling GC.SuppressFinalize (inside SkiaSharp)

What errors did you get when disposing?


stroke.UpdatePaint(fillPaint, strokeGeometry.Bounds);

LastDrawnStrokePath = strokeGeometry;
Copy link
Member

Choose a reason for hiding this comment

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

Should we dispose the previous one here?

Comment on lines +17 to +18
internal SKPath? LastDrawnStrokePath { get; private set; }
internal SKPath? LastDrawnFillPath { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

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

Especially this they are no longer disposed.

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-16201/index.html

@github-actions github-actions bot removed the area/automation Categorizes an issue or PR as relevant to project automation label Apr 15, 2024
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16201/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-16201/index.html

@Youssef1313 Youssef1313 mentioned this pull request Apr 15, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/skia ✏️ Categorizes an issue or PR as relevant to Skia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HitTest should rely on Visual
4 participants