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

[VisualDiagnostics] Fix RectangleAdorner with updated math #8728

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

drasticactions
Copy link
Contributor

Description of Change

The UITools team uses its own SelectorAdorner (Using IAdorner) for rendering its adorners used in Visual Studio. This was originally kept in line with the existing RectangleAdorner but as we updated the math in MAUI and our own code, this control was left broken.

This PR fixes that, and syncs the two again with the proper logic for render adorners on iOS, Android, Mac Catalyst, and Windows.

NOTE: The current Adorners drawn in Visual Studio using the Live Visual Tree or XAML Live Preview are not affected by this PR and should be currently working in MAUI. This is only for the built in RectangleAdorner.

Issues Fixed

Fixes #8727

@mattleibow
Copy link
Member

I managed to fix this by simply not doing density calculations. Looking at the code, it appears you do 1/density and then later another /1/density which feels like you are working around the bug where you are not supposed to use density in the calculations at all.

The bounding box should be in scaled units and then you are to draw in scaled units.

@drasticactions
Copy link
Contributor Author

@mattleibow I was syncing what we had in UI tools with it here, which I assumed was right.

I was wrong with that assumption then, or this is a conversation to have with @etvorun since I thought we did need to care about density. If you wanna fix this by removing the density calculation, you can do that and close this PR. I'm more than happy to have less code than more in this case.

@mattleibow
Copy link
Member

mattleibow commented Jul 14, 2022

I do not believe the logic is wrong, but what I do see is a bunch of division that ultimately ends up in a very similar place. I think this may be due to the fact that there was a bug in GA that used device pixels instead of scaled pixels. That required half the work in the IDE. Then we fixed that and then maybe the IDE did the other half instead of just using values directly.

In the screenshot below, we got the bounding box and then in the update model logic it converts it to device units then back to scaled units and then does some math. You can see the value of box (direct from the UI) is almost the same as the final model.BoundingBox:

image

Model Direct
image image
public virtual void Draw(ICanvas canvas, RectF dirtyRect)
{
	canvas.FillColor = FillColor;
	canvas.StrokeColor = StrokeColor;

	var boundingBox = VisualView.GetBoundingBox();

	DrawnRectangle = boundingBox.Offset(Offset); // <- direct usage

	canvas.FillRectangle(DrawnRectangle);
}

@etvorun
Copy link
Contributor

etvorun commented Jul 15, 2022

@mattleibow this change is a fork of our WPF adorner logic. The only reason we need 1/density is to achieve pixel perfect 1px crisp lines (see use of RoundToPixels). Otherwise we use units only.

@mattleibow mattleibow merged commit 0779d0f into dotnet:net6.0 Jul 15, 2022
@samhouts samhouts added the area-tooling XAML & C# Hot Reload, XAML Editor, Live Visual Tree, Live Preview, Debugging label Jul 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
@samhouts samhouts added fixed-in-6.0.486 Look for this fix in 6.0.486 SR4! fixed-in-7.0.0-rc.1.6683 labels Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-tooling XAML & C# Hot Reload, XAML Editor, Live Visual Tree, Live Preview, Debugging fixed-in-6.0.486 Look for this fix in 6.0.486 SR4! fixed-in-7.0.0-rc.1.6683
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants