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

A thin wrapper for SDL_Rect #6814

Merged
merged 7 commits into from Jul 2, 2022
Merged

Conversation

mesilliac
Copy link
Contributor

Similar to #6813 in creating a rect wrapper for SDL_Rect.

There are several things i wanted this for that are unwieldy when dealing with SDL_Rect directly. Things like pixel scale operations where i want to do things like multiply and divide rects by scalars, and intersections and clipping which rapidly become unwieldy when calling functions on pairs of rects.

The way this is done makes it interchangeable with SDL_Rect in all current situations. Things can either switch to using rect, or not, with no change.

Usage of rectangle manipulation functions generally becomes decently cleaner using this. I moved the standalone functions to rect member functions, so sdl::point_in_rect(x, y, r) became r.contains(x, y), sdl::intersect_rects(a, b) became a.intersect(b) etc. Several usages were things like a = sdl::intersect_rects(a, b), which became a.clip(b) which modifies a in place. It doesn't look like all that much, but existing usage became a lot cleaner when i converted things over.

The messiest part is dealing with things that are calling their internal variables "rect". I only converted things that were using the rectangle manipulation functions, most things are still using and can keep using SDL_Rect.

This is based on top of #6813 because i used the SDL_Point inheritance to simplify a couple things. So the parts of that that are in this can be ignored... i just want to put this up as a PR now because i have so much mess in my local at the moment. Trying to do one thing has led to four separate cleanups.

@github-actions github-actions bot added Editor Map/scenario editor issues. Help In-game Help functions UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine. labels Jun 30, 2022
@CelticMinstrel
Copy link
Member

Looking at the rectangle class in OBoE, I see a few other functions that you don't have here, but some of them are only needed because that rect uses a two-points representation rather than point and size. However, this one might be useful for something? (I converted it to point-and-size representation.)

void rect::inset(int dh, int dv) {
	x += dh; w -= 2 * dh;
	y += dv; h -= 2 * dv;
}

Also, functions to calculate the corners and centre of the rectangle seem like they might be useful for aligning stuff.

Also, it might be worth adding streaming output operators. To all three, if the others don't already have one (which maybe they do, I didn't check).

@mesilliac
Copy link
Contributor Author

mesilliac commented Jul 1, 2022

Hmm, yeah i have noticed some places doing this manually, for example rectangle borders. Now that you mention it a lot of layout stuff would probably be simpler as rect member functions.

The biggest sticking point so far is figuring out a naming system that makes it obvious whether the object itself is being modified or whether it's creating a new object. It's obvious with a *= b vs a * b for example, but i had to make up distinct names for clip vs intersect before it felt both usable and clear. English doesn't seem to have a good way of distinguishing this.

I was considering shrink and grow for ones that modified the object size as the verbs make it clear the object itself is changing.

Maybe there is a good way of structuring the names to make it obvious? I tried a few name constructions but none seemed all that clear. I've seen other systems use <verb> for modification vs <adjective> for creation (like a.clip() vs a.clipped()) which can work, but it felt weird when i tried it with intersect...

@mesilliac
Copy link
Contributor Author

Oh and re: ostream operators yeah, looks like color_t and point already have them. I did add a compact one for rect. There is one for SDL_Rect as well, but it's very verbose and i find it a bit difficult to parse. I guess there should be a decision on how these should look? The color_t one also isn't very clear.

currently representations are

color_t(255, 60, 140, 255) --> "255 60 140 255"
point(10,25) --> "10,25"
SDL_Rect(3, 15, 22, 89) --> "x: 3, y: 15, w: 22, h: 89"
rect(3, 15, 22, 89) --> "[3,15|22,89]"

I guess i would prefer compact forms that made the type obvious somehow. Maybe "#RRGGBBAA" form for colours? And "(X,Y)" for points. I put "[X,Y|W,H]" for rects to make the width/height part distinct, which also makes it obvious it's a rect, but i actually forgot i had done that. Maybe i should make an issue for discussion?

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Jul 1, 2022

I quite like inset/outset, but shrink/grow works too. (Though technically you only need one function – outset is just inset with a negative amount.)

I guess i would prefer compact forms that made the type obvious somehow. Maybe "#RRGGBBAA" form for colours? And "(X,Y)" for points. I put "[X,Y|W,H]" for rects to make the width/height part distinct, which also makes it obvious it's a rect, but i actually forgot i had done that. Maybe i should make an issue for discussion?

Um, I would say the point should add a pair of surrounding parentheses, and the colour… yeah, that existing one is terrible. Hex notation seems reasonable; either that or something like rgba(r, g, b, a) (CSS syntax basically).

Your rect one is pretty good, I like it. My first instinct would've probably been to format it as a pair of pairs (((x,y), (w,h))) but I think I like yours better. (The pair-of-pairs representation would be better as ((left, top), (right, bottom)) anyway.)

@mesilliac
Copy link
Contributor Author

Yeah, i initially thought to format it as a pair of points, but had the confusion with size vs bottom-right corner position. There is actually a local rectangle implementation somewhere (the old theme system i think?) that specifies its rectangles by top-left pos and bottom-right pos.

I like "inset" as a version that returns a new rectangle. I think that's used more in the layout code than modifying an existing rect. But it does differ from the usage above, which could be confusing...

In any case i'm not too attached to any of the names i've used here. I'd be happy to have them renamed to better fit a naming scheme, or other existing frameworks, or general intuitive usage.

Currently it's only used to simplify construction and for a better
null rect check, but it will soon be expanded to do a little more.
Mostly useful for simplifying pixel scale conversions.
rect::intersect returns the constructed intersection, whereas
rect::clip modifies the rectangle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editor Map/scenario editor issues. Help In-game Help functions UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants