Hidden vectorio

[FoamyGuy]

resolves #6204

Here's a short recording showing the behavior with these changes running the reproducer code from the issue

hidden_vectorio_objs.mp4

[FoamyGuy]

Latest commit corrects an issue where vectorio shapes that were nested at least 2 layers deep inside of groups would not be properly hidden / shown if the outermost parent (or probably any parent except the one the shape was directly inside of) was the one set to hidden.

[FoamyGuy]

With a point in the right direction by WarriorOfWire I think the code from here is how this can be done in the more generic way without having to check for each possible type:

circuitpython/shared-module/displayio/Group.c

Lines 147 to 152 in 5f4f667

	const vectorio_draw_protocol_t *draw_protocol = mp_proto_get(MP_QSTR_protocol_draw, self->members->items[i]);
	if (draw_protocol != NULL) {
	layer = draw_protocol->draw_get_protocol_self(self->members->items[i]);
	draw_protocol->draw_protocol_impl->draw_update_transform(layer, &self->absolute_transform);
	continue;
	}

I'll come back to this and attempt something similar for the changes in this PR to minimize the amount of new code needed.

[kvc0]

With a point in the right direction by WarriorOfWire I think the code from here is how this can be done in the more generic way without having to check for each possible type:

circuitpython/shared-module/displayio/Group.c

Lines 147 to 152 in 5f4f667

	const vectorio_draw_protocol_t *draw_protocol = mp_proto_get(MP_QSTR_protocol_draw, self->members->items[i]);
	if (draw_protocol != NULL) {
	layer = draw_protocol->draw_get_protocol_self(self->members->items[i]);
	draw_protocol->draw_protocol_impl->draw_update_transform(layer, &self->absolute_transform);
	continue;
	}

I'll come back to this and attempt something similar for the changes in this PR to minimize the amount of new
code needed.

Some additional context for $future_reader:

By doing it the explicit type check way you exploit knowledge of the actual Draw Protocol implementation for each of the types (which can be changed!). This duplicates knowledge of the Draw Protocol types’ implementations outside of the types themselves. 😬 As an example of where this would add pain, I’m planning to make a python binding so people can implement whatever bonkers shapes they can imagine in Python. That would potentially not use VectorShape for the Draw Protocol implementation. Then there’s a pending bug “hidden does not work on PythonShape”!

Flexibility, forward compatibility and ease of maintenance is what Draw Protocol is for ❤️. Less code is icing on the cake, but the real value is in easier iteration on vectorio, where all the valuable ancillary features (like hidden!) “just work” with new additions. It’s like an interface in other languages - there’s a touch more ceremony in C but the micropython mp_proto_get() makes it neat and tidy, and coding to available interfaces is always worth the effort!

[FoamyGuy]

@WarriorOfWire First, thanks for the additional explanation this is all quite helpful. And the ability to define new shapes from python sounds awesome!

Looking into changing this I'm thinking I will need to expose the set_dirty() function in the protocol impl here:

circuitpython/shared-bindings/vectorio/__init__.h

Lines 22 to 28 in 5f4f667

	typedef struct _vectorio_draw_protocol_impl_t {
	draw_fill_area_fun draw_fill_area;
	draw_get_dirty_area_fun draw_get_dirty_area;
	draw_update_transform_fun draw_update_transform;
	draw_finish_refresh_fun draw_finish_refresh;
	draw_get_refresh_areas_fun draw_get_refresh_areas;
	} vectorio_draw_protocol_impl_t;

Does that seem right to you?

I made a first attempt at it and wasn't successful but am going to get back into it and try some more. Wanted to make sure I was headed down the right path though before I go too far.

[kvc0]

@FoamyGuy yeah, that sounds right! Probably the right thing to do is to replace on_dirty with your new draw protocol method too. https://github.com/adafruit/circuitpython/blob/main/shared-module/vectorio/Polygon.c#L198

[FoamyGuy]

I got this working, but unsure if I've gone about it in the correct way.

I didn't end up making a new function I don't think, I'm not sure how to set the on_dirty event

[FoamyGuy]

I think this is ready now. I've tested several more scenarios around it and everything has worked as expected so far.

Am interested in getting clarification from @WarriorOfWire about what possible issues may still exist as well as feedback from anyone else.

[kvc0]

This looks great to me. Thank you for sticking with this change!

[dhalbert]

@tannewt I have not followed this. Do you want to review before merging?

[tannewt]

I just skimmed it and it looks good. Thanks to @WarriorOfWire for the main review.