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

implemented grow(left,top,right,bottom) function #8981

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented May 29, 2017

this function is useful when resizing a rect with different values for each corner.
I need it for the new rounded corner style box implementation.

since it is not directly related with the style box, it might be nicer to have a separated pr...

@bojidar-bg
Copy link
Contributor

Mind if you bind it to GDScript?

Also, not sure if we should have a grow_margin(int(MARGIN_*) margin, real_t amount), WDYT?

@toger5
Copy link
Contributor Author

toger5 commented May 29, 2017

grow margin also would be nice!
I think those it's really convenient to have those kind of rect conversions.
should I overload the function or use different names:

.grow(real_t amount)
.grow(left,top,right,bottom)
.grow(Margin:margin, real_t amount)

OR

.grow(real_t amount)
.grow_individual(left,top,right,bottom) //that sounds stupid...
.grow_margin(Margin:margin, real_t amount)

@akien-mga
Copy link
Member

I think overloaded functions look better. For the second one, it would likely be better to use the same convention as in CSS (top, right, bottom left), unless there's already a precedent in Godot's API for starting with left.

@toger5
Copy link
Contributor Author

toger5 commented May 29, 2017

order was inspired by margin:

enum Margin {

	MARGIN_LEFT,
	MARGIN_TOP,
	MARGIN_RIGHT,
	MARGIN_BOTTOM
};

okay sounds good. (overloading)
I won't do the bindings today (5am...) and the additional (margin,amount) function
where would I find them. (They are not in the Rect2 class)

@akien-mga
Copy link
Member

Ok, you can keep the same order as in the enum then, it makes sense.

@bojidar-bg
Copy link
Contributor

I'm not sure if we can bind the overloaded methods properly though...

@toger5
Copy link
Contributor Author

toger5 commented May 29, 2017

I'm currently trying... could not compile yet...
If I can't bind overloaded funcs, what naming convention?

grow_margin
grow
grow_expand(left, top,right,bottom) / grow_individual / grow_all / grow_borders / ...

@toger5
Copy link
Contributor Author

toger5 commented May 29, 2017

okay same name is just overwriting the binding -> ony one grow function possible...

@toger5
Copy link
Contributor Author

toger5 commented May 29, 2017

@akien-mga @bojidar-bg the requested updates are done.

@toger5
Copy link
Contributor Author

toger5 commented Jun 5, 2017

@akien-mga merge ?
or do you want me to rebase before?

@akien-mga
Copy link
Member

Would be good if you can squash as the second commit modifies the first commit's implementation. You can take the opportunity to fix the typo in the commit log :p

@toger5
Copy link
Contributor Author

toger5 commented Jun 5, 2017

good call

 - grow_individual
 - grow_margin
@toger5 toger5 force-pushed the addedGrowFunction branch from 8296e8d to 66b3089 Compare June 5, 2017 08:11
@toger5
Copy link
Contributor Author

toger5 commented Jun 5, 2017

rebased

@akien-mga akien-mga added this to the 3.0 milestone Jun 5, 2017
@akien-mga akien-mga merged commit bee81d5 into godotengine:master Jun 8, 2017
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.

3 participants