Skip to content

Make draw_gouraud_triangle optional and remove unused versions #23820

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

Closed
wants to merge 1 commit into from

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Sep 7, 2022

PR Summary

Closes #23819

Clarify that it is enough to implement one of draw_gouraud_triangle and draw_gouraud_triangles in a backend and that any Artist should call the latter.

Remove unused versions of draw_gouraud_triangle.

I assume that there should be some sort of note here clarifying the changed behaviour (in practice that any custom artists should call draw_gouraud_triangles). Including the example code:

    def draw_gouraud_triangle(self, gc, points, colors, trans):
        self.draw_gouraud_triangles(gc, points.reshape((1, 3, 2)),
                                    colors.reshape((1, 3, 4)), trans)

I do not really see a way to deprecate this, without really deprecating draw_gouraud_triangle completely (and instead introduce something like _draw_gouraud_triangle which wouldn't really fit well with the general structure). It would be possible to deprecate the now removed versions though. A quick search show that https://github.com/ArduPilot/MAVProxy uses a copy of the backend_agg.py-file which has it included, but that is the only thing I could find which may cause anyone some problem.

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@anntzer
Copy link
Contributor

anntzer commented Sep 7, 2022

I would suggest just deprecating the singular version to keep things simple.
If you are writing a backend that can draw gouraud triangles (a rather complex operation), hopefully doing the looping yourself is not too much of an additional complication.

@timhoffm
Copy link
Member

timhoffm commented Sep 7, 2022

If I understand @anntzer correctly, I agree. The public API should be the plural (because that allows backend implementers to push everything low-level and thus not have an expensive python loop - if they can; if they can't they write the python loop in the plural function and call a single-drawing function in there).

@oscargus
Copy link
Member Author

oscargus commented Sep 8, 2022

Yeah, I agree that it makes sense to deprecate the singular function. I sort of thought that "if one can implement one Gouraud triangle draw routine it should be enough", but indeed the overhead of writing a loop is rather small in that situation.

I'll open a new PR for that I think, rather than unrolling everything in this, and then we can decide.

@oscargus oscargus marked this pull request as draft September 8, 2022 08:53
@oscargus oscargus mentioned this pull request Sep 8, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MNT]: Make draw_gouraud_triangle optional
3 participants