Added text_color parameter for VertexLabelAnnotator

[Bhavay-2001]

Description

Part of #1384

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

How has this change been tested, please provide a testcase or example of how you tested the change?

1. Notebook

Any specific deployment considerations

For example, documentation changes, usability, usage/costs, secrets, etc.

Docs

• Docs updated? What were the changes:

[Bhavay-2001]

Hi @SkalskiP, I have made the changes as per my understanding. In the VertexLabel class, it is running a for loop for different colors where there should be for text color too here

Soo I think it should add some function for adding multiple text_colors

[Bhavay-2001]

Hi @SkalskiP @LinasKo pls review this PR.
Thanks

[SkalskiP]

Hi, @Bhavay-2001 👋🏻 Thanks a lot for your interest in supervision. It Looks like you added external API and docstring, but it looks like changing the value of text_color does not result in changing the text color per anchor :/ Am I missing something?

[Bhavay-2001]

Hi @SkalskiP, Yess, this was little bit incomplete.

The color parameter of the VertexLabelAnnotator is Union[Color, List[Color]]. Should we add this in text_color as well soo that we can use the same logic as in the color parameter?

[SkalskiP]

Hi @Bhavay-2001 👋🏻 I'd even say we should make it the same as other annotators and make both color and text_color to be Union[Color, ColorPalette].

[Bhavay-2001]

Alright. Then, maybe we should take the this logic from other annotators.

[SkalskiP]

If it's possible to reuse some parts of that logic, then sure.

[Bhavay-2001]

Before making the changes, I would like to discuss regarding this.

Currently, the color parameter in VertexLabelAnnotator accepts either single color or a list. If we change it to ColorPallette, then we need to make changes to this function -

def preprocess_and_validate_colors(
        colors: Optional[Union[Color, List[Color]]],
        points_count: int,
        skeletons_count: int,
    ) -> np.array:
        if isinstance(colors, list) and len(colors) != points_count:
            raise ValueError(
                f"Number of colors ({len(colors)}) must match number of key points "
                f"({points_count})."
            )
        return (
            np.array(colors * skeletons_count)
            if isinstance(colors, list)
            else np.array([colors] * points_count * skeletons_count)
        )

In this, we need to change the color parameter in function definition and then it will return a list of colors. Should I proceed with this?

[SkalskiP]

@Bhavay-2001, after further consideration, let's not change the type for now. Keep color as either Color or List[Color] (let's not introduce ColorPalette yet). But let's make text_color also Color or List[Color].

[Bhavay-2001]

Hi @SkalskiP, please check the code and the notebook.
Thanks

[LinasKo]

Hi @Bhavay-2001,

I left one comment in the code.

You've also added 2 images. Please remove those.

[Bhavay-2001]

@LinasKo, Should I call the preprocess_and_validate_colors method here too to get the list of text_colors then?

[Bhavay-2001]

Done

[LinasKo]

this should be as_bgr. OpenCV has a blue-green-red ordering of colors. You can find that we call other opencv functions similarly.

[Bhavay-2001]

Yes right. I have changed this code some place else too. Missed here sorry.

[LinasKo]

Hi @Bhavay-2001, I've now approved the pull request and will merge it in. Thank you for the contribution.

However, it is also an example of a contribution we won't accept in the future. For a feature this small, the number of times we had to come back to verify, the number of mistakes we found, the number of questions we had to answer - that takes 5x time than it saves, especially for a feature this small. Looking back, this also holds true for #1387 and #1227.