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

Extend CanvasItem::draw_circle(), making it also draw unfilled circle. #84472

Merged

Conversation

xiongyaohua
Copy link
Contributor

@xiongyaohua xiongyaohua commented Nov 5, 2023

func _draw():
	draw_circle(Vector2(0,200), 200, Color.BROWN)
	draw_circle(Vector2(300,200), 200, Color.BROWN, false)
	draw_circle(Vector2(600,200), 200, Color.BROWN, false, 10)

image

@xiongyaohua xiongyaohua requested review from a team as code owners November 5, 2023 04:29
@xiongyaohua xiongyaohua changed the title Extend 'CanvasItem::draw_circle()', making it possible to draw unfilled circle, like 'draw_rect()'. Extend CanvasItem::draw_circle(), making it possible to draw unfilled circle, like draw_rect(). Nov 5, 2023
@xiongyaohua xiongyaohua changed the title Extend CanvasItem::draw_circle(), making it possible to draw unfilled circle, like draw_rect(). Extend CanvasItem::draw_circle(), making it also draw unfilled circle. Nov 5, 2023
@AThousandShips AThousandShips added this to the 4.x milestone Nov 5, 2023
@AThousandShips
Copy link
Member

Please see this

@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 5, 2023

Random thought but already too late for it, I noticed that the circle-drawing pipelines could easily be extended for ellipses without any overhead. I'll try to remember to bring this up for 5.0

Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

Looks good to me.

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved
@MewPurPur
Copy link
Contributor

Also consider the suggestion for adding an antialiased parameter, and documenting that it only works for unfilled.

@xiongyaohua xiongyaohua force-pushed the canvas_item_draw_circle_non_filled branch from e910b0c to 18493f3 Compare November 5, 2023 12:48
@xiongyaohua
Copy link
Contributor Author

Also consider the suggestion for adding an antialiased parameter, and documenting that it only works for unfilled.

Beside this PR I find there are several other inconsistencies in the Canvasitem draw API. I am going to make a proposal then address them in an following PR.

@AThousandShips
Copy link
Member

AThousandShips commented Nov 5, 2023

This was not necessarily a suggestion in the proposal, so I'd say it should be added now instead of later

And new functionality shouldn't add to the inconsistencies IMO

@xiongyaohua
Copy link
Contributor Author

xiongyaohua commented Nov 5, 2023

Let me explain. The inconsistencies include:

  1. if there is p_antialiased for draw_circle, then there should also be p_antialiased for draw_rect.
  2. if p_antialiased works for unfilled version of circle or rect, then it should works for filled versions.
  3. if p_antialiased works for line and polyline, then it should works for dashed_line and multiline

I plan to address them step by step, instead of mixed all things in one big PR. The plan is:

  • next PR: add p_antialiased for unfilled version of both circle and rect. If p_antialiased is add to circle in current PR, then we are in the position that the circle support antialias while the rect doesn't. So I prefer to add it to both in the next PR.
  • then: make p_antialiased works for filled circle and rect, using the same "feather" method as the antialiased line
  • then: add p_antialiased to dashed_line and multiline

edit:

@MewPurPur
Copy link
Contributor

OK! I look forward. I think this is good as is.

@MewPurPur
Copy link
Contributor

Actually, maybe @dalexeev has something to say about this, having worked on draw_rect() a while back. I remember daleexev struggling to figure out whether increasing width should make the rect's stroke should grow inward, outward, or in both directions.

@Calinou
Copy link
Member

Calinou commented Nov 6, 2023

Random thought but already too late for it, I noticed that the circle-drawing pipelines could easily be extended for ellipses without any overhead. I'll try to remember to bring this up for 5.0

This can be done in a backwards-compatible way by adding draw_ellipse(), but it should be discussed in its own proposal.

doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
scene/main/canvas_item.cpp Outdated Show resolved Hide resolved
@xiongyaohua
Copy link
Contributor Author

The Mono build always give error, because of some API mismatching. But I am not sure how should I proceed to solve it, can someone shed some light?

@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 7, 2023

It's fine, you don't need to change anything. Edit: Nevermind, sorry

@xiongyaohua
Copy link
Contributor Author

xiongyaohua commented Nov 7, 2023

It's fine, you don't need to change anything.

Good to know, thanks!

@AThousandShips
Copy link
Member

It's fine, you don't need to change anything.

This is false, you need to provide compatibility methods, this is for GDExtension, this is required when ever additional arguments are added, take a look in other files which have a .compat.inc file associated with them

scene/main/canvas_item.cpp Outdated Show resolved Hide resolved
scene/main/canvas_item.cpp Outdated Show resolved Hide resolved
@xiongyaohua xiongyaohua force-pushed the canvas_item_draw_circle_non_filled branch from eb1e30e to 88e7471 Compare November 8, 2023 03:43
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, will need some dedicated render review to be sure but it matches the general method for this

Just some minor nitpicks

misc/extension_api_validation/4.1-stable.expected Outdated Show resolved Hide resolved
misc/extension_api_validation/4.1-stable.expected Outdated Show resolved Hide resolved
scene/main/canvas_item.h Outdated Show resolved Hide resolved
@xiongyaohua xiongyaohua force-pushed the canvas_item_draw_circle_non_filled branch from 700f56e to e281cd1 Compare November 9, 2023 01:31
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

I suggest adding a final antialiased parameter that defaults to false, and passes this parameter to canvas_item_add_polyline().

@AThousandShips
Copy link
Member

AThousandShips commented Nov 10, 2023

That is currently handled in a separate follow-up PR:

But I'd agree that doing it in one PR is most convenient and makes it less cumbersome compatibility wise

@xiongyaohua xiongyaohua force-pushed the canvas_item_draw_circle_non_filled branch 2 times, most recently from f66f4ae to 012e43c Compare November 30, 2023 14:39
@xiongyaohua
Copy link
Contributor Author

I suggest adding a final antialiased parameter that defaults to false, and passes this parameter to canvas_item_add_polyline().

done

@xiongyaohua xiongyaohua closed this Dec 2, 2023
@xiongyaohua xiongyaohua reopened this Dec 2, 2023
@xiongyaohua
Copy link
Contributor Author

xiongyaohua commented Dec 2, 2023

@AThousandShips Might this PR be considered to be moved to 4.3? The change is relatively straightforward and small in scope, yet the improve in user convenience is considerable.

@AThousandShips
Copy link
Member

AThousandShips commented Dec 2, 2023

No rush 🙂 it still needs some further review by render the render team before it can be decided on merger

This addition might be straightforward, but we need to be careful about how we decide to go with it so we don't have to change it and break compatibility in the future, so a thought through decision is needed

@xiongyaohua
Copy link
Contributor Author

Fair enough, thanks for the explanation.

Copy link
Member

Choose a reason for hiding this comment

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

Should now be moved to the 4.2 file since it exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@xiongyaohua xiongyaohua force-pushed the canvas_item_draw_circle_non_filled branch from 012e43c to 7f98430 Compare December 24, 2023 02:41
@akien-mga akien-mga requested a review from a team January 4, 2024 11:02
@xiongyaohua xiongyaohua force-pushed the canvas_item_draw_circle_non_filled branch from 7f98430 to 701ad73 Compare February 21, 2024 10:17
@xiongyaohua xiongyaohua force-pushed the canvas_item_draw_circle_non_filled branch 3 times, most recently from d0941d9 to a9b157d Compare February 21, 2024 10:49
@akien-mga akien-mga requested a review from Calinou February 21, 2024 14:13
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 6118592), it works as expected in all rendering methods, with and without HDR 2D enabled.

Testing project: test-draw-circle.zip

output_forward_plus

@akien-mga
Copy link
Member

Needs a rebase, and then this should be mergeable.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Just a minor detail

doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
…d options

Make it possible to draw unfilled circle, like draw_rect(). Antialising is only implemented for unfilled version.
@akien-mga akien-mga force-pushed the canvas_item_draw_circle_non_filled branch from a9b157d to 1f2aa17 Compare May 2, 2024 08:43
@akien-mga akien-mga merged commit bbb8667 into godotengine:master May 2, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jun 12, 2024
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.

Add filled and width parameters to draw_circle
6 participants