Skip to content

displayio and vectorio: move to displayio_area_union and away from _expand #4486

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.

Sign up for GitHub

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 2 commits into from
Mar 27, 2021
Merged

displayio and vectorio: move to displayio_area_union and away from _expand #4486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9485805
move to displayio_area_union and away from _expand
kmatch98 Mar 25, 2021
56362a9
remove duplicated code, replace with displayio_area_canon
kmatch98 Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions shared-module/displayio/Group.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,15 @@ bool displayio_group_get_previous_area(displayio_group_t *self, displayio_area_t
displayio_area_copy(&layer_area, area);
first = false;
} else {
displayio_area_expand(area, &layer_area);
displayio_area_union(area, &layer_area, area);
}
}
if (self->item_removed) {
if (first) {
displayio_area_copy(&self->dirty_area, area);
first = false;
} else {
displayio_area_expand(area, &self->dirty_area);
displayio_area_union(area, &self->dirty_area, area);
}
}
return !first;
Expand Down Expand Up @@ -307,7 +307,7 @@ static void _remove_layer(displayio_group_t *self, size_t index) {
if (!self->item_removed) {
displayio_area_copy(&layer_area, &self->dirty_area);
} else {
displayio_area_expand(&self->dirty_area, &layer_area);
displayio_area_union(&self->dirty_area, &layer_area, &self->dirty_area);
}
self->item_removed = true;
}
Expand Down
2 changes: 1 addition & 1 deletion shared-module/displayio/TileGrid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ void common_hal_displayio_tilegrid_set_tile(displayio_tilegrid_t *self, uint16_t
tile_area->y2 = tile_area->y1 + self->tile_height;

if (self->partial_change) {
displayio_area_expand(&self->dirty_area, &temp_area);
displayio_area_union(&self->dirty_area, &temp_area, &self->dirty_area);
}

self->partial_change = true;
Expand Down
15 changes: 0 additions & 15 deletions shared-module/displayio/__init__.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,21 +245,6 @@ void displayio_gc_collect(void) {
}
}

void displayio_area_expand(displayio_area_t *original, const displayio_area_t *addition) {
Copy link

Choose a reason for hiding this comment

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

Did you consider or measure the effective impact of switching from 4 conditioned writes to multiple conditions followed by 4 branching unconditional writes?
Probably in this case it is okay because it is per-frame, but seemingly innocuous change can have dramatic pixel fill rate or frame latency impact in this area so I’m somewhat skeptical of doing more work than is necessary.

Copy link
Author

Choose a reason for hiding this comment

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

That's a great point. My main motivation was to fix an issue where when area1 was combined with an empty area (defined as empty by x1==x2==0) that it would return an area with x1=0 and x2=area1.x2. This caused unnecessary dirty rectangle redrawing.

But your point is about performance of the union command vs. the code in expand. I didn't look at any performance differences. I've never done any performance testing down at the C-level. If you give a good example of how best to do this to make the comparison, and I'll check it out.

Copy link

Choose a reason for hiding this comment

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

I think a test for this would be a chore. Native time functions have a 32khz resolution iirc and either call should surely take less than 31 microseconds? You could time a frame with several 1px overlapping rectangles but there’s quite a bit of noise in that aggregate signal. I’ve wrapped code up higher in the displayio draw stack to A/B test things in this way but I do not have a clear example for you to follow.
Last time I was actively adding to vectorio I wanted to make a more convenient way to use a high precision hardware timer for things like this.
I really don’t think there’s a problem with this code, I just appreciate you taking an active interest and hope you also always defend (and improve!) the pixel fill rate :-)

if (addition->x1 < original->x1) {
original->x1 = addition->x1;
}
if (addition->y1 < original->y1) {
original->y1 = addition->y1;
}
if (addition->x2 > original->x2) {
original->x2 = addition->x2;
}
if (addition->y2 > original->y2) {
original->y2 = addition->y2;
}
}

void displayio_area_copy(const displayio_area_t *src, displayio_area_t *dst) {
dst->x1 = src->x1;
dst->y1 = src->y1;
Expand Down
1 change: 0 additions & 1 deletion shared-module/displayio/area.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ void displayio_area_canon(displayio_area_t *a);
void displayio_area_union(const displayio_area_t *a,
const displayio_area_t *b,
displayio_area_t *u);
void displayio_area_expand(displayio_area_t *original, const displayio_area_t *addition);
void displayio_area_copy(const displayio_area_t *src, displayio_area_t *dst);
void displayio_area_scale(displayio_area_t *area, uint16_t scale);
void displayio_area_shift(displayio_area_t *area, int16_t dx, int16_t dy);
Expand Down
13 changes: 2 additions & 11 deletions shared-module/vectorio/VectorShape.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,7 @@ static void _get_screen_area(vectorio_vector_shape_t *self, displayio_area_t *ou
out_area->y2 = (out_area->y2 + self->y) * self->absolute_transform->dy + self->absolute_transform->y;
}
// We might have mirrored due to dx
if (out_area->x2 < out_area->x1) {
int16_t swap = out_area->x1;
out_area->x1 = out_area->x2;
out_area->x2 = swap;
}
if (out_area->y2 < out_area->y1) {
int16_t swap = out_area->y1;
out_area->y1 = out_area->y2;
out_area->y2 = swap;
}
displayio_area_canon(out_area);
VECTORIO_SHAPE_DEBUG(" out:{(%5d,%5d), (%5d,%5d)}\n", out_area->x1, out_area->y1, out_area->x2, out_area->y2);
}

Expand All @@ -88,7 +79,7 @@ void common_hal_vectorio_vector_shape_set_dirty(void *vector_shape) {
self->ephemeral_dirty_area.x1, self->ephemeral_dirty_area.y1, self->ephemeral_dirty_area.x2, self->ephemeral_dirty_area.y2);
self->dirty = true;
// Dirty area tracks the shape's footprint between draws. It's reset on refresh finish,
displayio_area_expand(&self->ephemeral_dirty_area, &current_area);
displayio_area_union(&self->ephemeral_dirty_area, &current_area, &self->ephemeral_dirty_area);
VECTORIO_SHAPE_DEBUG(" -> expanded:{(%3d,%3d), (%3d,%3d)}\n", self->ephemeral_dirty_area.x1, self->ephemeral_dirty_area.y1, self->ephemeral_dirty_area.x2, self->ephemeral_dirty_area.y2);
}

Expand Down