Skip to content

Adopt DensityValue in Grid to Enable Precise Pixel-Aware Layout#30020

Closed
Copilot wants to merge 46 commits into
mainfrom
copilot/fix-30017
Closed

Adopt DensityValue in Grid to Enable Precise Pixel-Aware Layout#30020
Copilot wants to merge 46 commits into
mainfrom
copilot/fix-30017

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jun 17, 2025

Issues Fixed

Fixes #28117

This PR implements the DensityValue proposal to improve Grid layout precision across density-independent units (dp) and ensure pixel-aligned rendering. It addresses layout inconsistencies caused by fractional pixel results, especially in high-DPI environments where evenly dividing space can lead to rounding errors.

Problem

In high-DPI environments, dividing space equally often results in fractional pixels that don't map cleanly to integers:

// Example: 293.4dp at density 2.625 = 770.175px across 3 columns
// Naive division: 770.175 / 3 = 256.725px per column  
// Independent rounding: 257 + 257 + 257 = 771px (1px overflow!)

This causes:

  • Layout gaps or overflow
  • Jittery rendering
  • Clipped visuals
  • Inconsistent star (*) sizing behavior

Solution

1. DensityValue Struct

Introduces an internal DensityValue struct that tracks both dp and pixel values:

internal readonly struct DensityValue
{
    public double Dp => RawPx / Density;
    public double Density { get; }
    public double RawPx { get; }
    
    // Distributes pixels with error accumulation like Android
    public static int[] DistributePixels(double totalPixels, double density, double[] portions)
}

2. Enhanced Grid Layout

Modifies GridLayoutManager.ResolveStars() to use density-aware distribution when available, falling back to the original algorithm when density information is unavailable.

3. Pixel-Perfect Distribution

The DistributePixels method implements Android's approach of accumulating rounding errors and assigning remainder pixels to the final elements:

// 293.4dp × 2.625 density = 770.175px across 3 equal columns
// Result: [256, 257, 257] pixels (total: 770px) ✓
// Instead of: [257, 257, 257] pixels (total: 771px) ❌

Validated Scenarios

All scenarios from the original issue are now handled correctly:

  • Scenario 1: 293.4dp across 3 columns (density: 2.625) → [256, 257, 257] pixels
  • Scenario 2: 290dp across 3 columns (density: 3.0) → [290, 290, 290] pixels
  • Scenario 3: 300dp across 4 columns (density: 2.625) → [196, 197, 197, 197] pixels
  • Scenario 4: 293dp across 4 columns (density: 2.75) → [201, 201, 201, 202] pixels

Benefits

  • ✅ Eliminates layout jitter from float rounding
  • ✅ Ensures exact pixel totals with no overflow
  • ✅ Improves star-based layout accuracy
  • ✅ Aligns with native Android/iOS layout behavior
  • ✅ Maintains full backward compatibility

Implementation Details

  • Internal API: DensityValue is internal, no public API changes
  • Minimal scope: Only 444 lines added, 0 deleted across 4 files
  • Fallback behavior: Uses original algorithm when density unavailable
  • Comprehensive tests: Validates all scenarios including weighted star sizing

Fixes #30017.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@PureWeen PureWeen self-assigned this Jun 17, 2025
Copilot AI changed the title [WIP] Proposal: Adopt DensityValue in Grid to Enable Precise Pixel-Aware Layout Adopt DensityValue in Grid to Enable Precise Pixel-Aware Layout Jun 17, 2025
Copilot AI requested a review from mattleibow June 17, 2025 01:47
@rmarinho
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Copy Markdown
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Make sure to always revert any changes that you make to json files that are in the Templates/src directory.

Please update your copilot instructions to help you with this next time.
We never want to commit any of the json files that get modified while copilot is working on a PR

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jun 17, 2025

Make sure to always revert any changes that you make to json files that are in the Templates/src directory.

Please update your copilot instructions to help you with this next time.
...

I've reverted all the JSON files in the Templates/src directory and updated the copilot instructions to include specific guidance about never committing these files. Commit 10a3883 addresses this feedback.

Copilot AI requested a review from PureWeen June 17, 2025 22:37
Comment thread src/Core/src/Layouts/GridLayoutManager.cs Outdated
Copy link
Copy Markdown
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Remove this extention Method

use the "GetWindow" extention method against the IGridLayout that's inside GridLayoutManager

That will give you an IWindow

You can retrieve the RequestDisplayDensity from that

If the window is null just use 1 for the DisplayDensity

Comment thread src/Core/src/WindowExtensions.cs Outdated
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jun 18, 2025

Remove this extention Method

use the "GetWindow" extention method against the IGridLayout that's inside GridLayoutManager
...

Updated to use window.RequestDisplayDensity() instead of creating a new extension method. Commit 16d2cd8 removes the custom extension method and uses the existing IWindow property as requested.

Copilot AI requested a review from PureWeen June 18, 2025 00:01
Copy link
Copy Markdown
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Remove all changes made to WindowExtention.cs

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jun 18, 2025

Remove all changes made to WindowExtention.cs

Removed all changes to WindowExtensions.cs as requested. Commit f6adcd9 reverts the file to its original state.

Copilot AI requested a review from PureWeen June 18, 2025 00:14
Copy link
Copy Markdown
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Can you add some unit tests to GridLayoutManagerTests that show how this works. Also, use some of the examples form the PR description.

Copilot AI and others added 28 commits June 30, 2025 16:28
…on pixel precision

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…ate copilot instructions

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
- Modified DensityValue to store pixels as primary value (RawPx) with Dp as computed property
- Updated Definition.Size to use DensityValue instead of double to maintain pixel precision
- Modified TryResolveStarsWithDensityAwareness to assign DensityValue directly from pixels without precision loss
- Updated all Definition usages throughout GridLayoutManager to use .Dp when dp values needed
- Added density parameter to static methods to enable DensityValue creation
- Eliminates precision loss from pixelAllocations[i] / density conversion

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
- Finalized GetDisplayDensity implementation to handle cases where GetHostedWindow extension method is not available
- Maintained IViewWithWindow interface for testing scenarios with proper density injection
- All calculations in GridLayoutManager now use pixels as source of truth to eliminate precision loss
- TryResolveStarsWithDensityAwareness assigns DensityValue directly from pixel allocations without conversion

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…x star resolution control flow

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…l arrangement

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…for pixel-precise calculations

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…enting future commits

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
… is 1.0

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…l distribution, and update test expectations

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
… JSON files

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…test infrastructure

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…test by preserving fractional precision when density=1.0

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…ck - remove TryExpandStarDefinitionsWithDensityAwareness and fix ResolveStars for loop placement

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…xpansion

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
… minimal view sizes, Math.Floor for pixel calculations, and dynamic expected value calculation

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…e type replacement

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…with proper backward compatibility

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…Completion Checklist

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…ty=1.0

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
…fect distribution

Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

4 participants