Skip to content
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

Added Canvas.DrawImage (#784) #878

Merged
merged 3 commits into from
Dec 27, 2020

Conversation

SirePi
Copy link
Member

@SirePi SirePi commented Oct 17, 2020

As per #784, this introduces a DrawImage(Material / BatchInfo) to the Canvas.
The code has been taken inspiration from FillRect, but feel free to add suggestions on how it can be improved

@SirePi SirePi requested a review from a team October 17, 2020 08:28
@SirePi SirePi added hacktoberfest-accepted Nice2Have Beneficial, but only very slightly so Rendering Related to rendering / graphics labels Oct 17, 2020
Source/Core/Duality/Drawing/Canvas.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Drawing/Canvas.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Drawing/Canvas.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Drawing/Canvas.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Drawing/Canvas.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Drawing/Canvas.cs Outdated Show resolved Hide resolved
@ilexp ilexp self-requested a review December 27, 2020 16:25
@ilexp
Copy link
Member

ilexp commented Dec 27, 2020

Thanks! Finally managed to look into this again and update the review.

Instead of requesting a few more changes, I took the time to add them myself right away, to avoid going stale again, since I think this is a pretty useful feature. Some of the changes were not part of earlier review requests and only occurred to me when doing a more thorough review today, checking out the most recent changes. I know it's not the cleanest way to go through with this PR, but right now it's the best I can manage, given the limited time I have - sorry about that, but I hope that's fine with you.

Changes I made and rationale behind them:

  • Premise: DrawImage is a shortcut that aims specifically to fit the "I want to draw this texture as-is" use case, since all others are better off with existing, more complex / verbose API.
  • Removed all ContentRef<Material> and BatchInfo overloads, since they are easily available via canvas.State.SetMaterial if needed. "Specifying a full material is already beyond the shortcut path that this should address."
  • Removed all overloads that allow to specify with and height, since they're already available via canvas.FillRect when setting the material beforehand. "Specifying an explicit size is already beyond the shortcut path that this should address."
  • Added a unit test for DrawImage.

@ilexp ilexp merged commit 635d922 into AdamsLair:master Dec 27, 2020
@ilexp ilexp linked an issue Dec 27, 2020 that may be closed by this pull request
This was referenced Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Nice2Have Beneficial, but only very slightly so Rendering Related to rendering / graphics
Development

Successfully merging this pull request may close these issues.

Canvas.DrawImage
2 participants