search

earlygrey / shapedrawer

A library for libGDX that draws simple shapes using a batch.
MIT License
187 stars 31 forks source link

ISideEstimator / DefaultSideEstimator #47

Closed TCreutzenberg closed 3 years ago

TCreutzenberg commented 3 years ago

This is my suggestion for the issue addressed by @payne911 in https://github.com/earlygrey/shapedrawer/issues/11 + https://github.com/earlygrey/shapedrawer/pull/46

Regarding AbstractShapeDrawer#estimateSidesRequired() there are different needs for different projects.

I guess what we all can agree on is that there needs to be a way to specify estimateSidesRequired(radiusX, radiusY) more precise or to offer ways to access different presets out of the box for different needs.

My suggestion is to have an ISideEstimator interface and have the possibility to easily change this in AbstractShapeDrawer. By default the DefaultSideEstimator is set. This can be easily replaced (even between render calls) or injected in the constructor. There's a lot of people who will not deep-dive into a library to adjust it to their needs. And having this might make everything easier to understand.

If somebody has a good proposal for other ISideEstimators they can also just directly be included here in this project without affecting any existing implementations.

(If this would get merged it should be a squash-merge, sorry for the amount of commits)

payne911 commented 3 years ago

Interesting implementation. It's nice to provide more flexibility while still maintaining backward compatibility.

Nonetheless, I would still advocate for more sides by default. Unless the "4x" is proven to have significant performance impact, I don't see why we would refrain from providing circles as circles by default.

The current heuristic just doesn't fulfill the contract of drawing a circle: it draws a polygon that resembles a circle.

earlygrey commented 3 years ago

@payne911 Are you calling drawer.update() after setting the projection matrix on the batch, or setting the pixel size? I don't think I've ever had a problem with the roundness of circles in various projects, and in the demo it looks fine to me. If it's really not working for you perhaps it's to do with the calculation not working well with your world unit/pixel ratio, in which case it would make more sense to adjust that part of the calculation rather than uniformly multiply the total by a constant, which might inadvertently affect performance on existing projects.

I'm in general fine with this PR, though I'm not a fan of the "I" prefix on interfaces and "multiplicator" should probably be "multiplier".

TCreutzenberg commented 3 years ago

@earlygrey Your feedback is implemented.

@payne911 May I ask what your usecase and setup is? Because I do not have a problem with the "roundness" of the circles. Maybe we can find the root of the artefact you are experiencing and solve it. My monitor is 2560x1440 and everything looks great in fullscreen. My world units are 3840x2160 and I'm drawing circles from about 200-4200 world units radius (400-8400 diameter). That is about 133-2800 pixels radius. I do not rotate the circles.

payne911 commented 3 years ago

I've been testing my library a little bit more to try to showcase this "lack of roundness" and was unable to reproduce it in any meaningful way. My comment and PR should pretty much be disregarded.

I think what I had in mind was actually more related to pixels being pixels, since it's tweaking the antialiasing samples number that made the arcs look smoother.

While this is an interesting proposal, it would only be a "hacky patch" for #11. If this is merged, I do not think #11 should be closed.

earlygrey commented 3 years ago

@TCreutzenberg would you still like to see this merged? If so it looks fine to me, though I'm not sure what problem it's solving now!

TCreutzenberg commented 3 years ago

Hm, I guess it's up to you if you want this merged or not, I'm fine with closing this PR or merging it.

If closed: I can just implement this or similar functionality myself by overriding estimateSidesRequired(float radiusX, float radiusY). If merged: This functionality is configurable out of the box.

ITt's not colving a problem, it's basically just adding functionality to change the behavour of estimateSidesRequired(float radiusX, float radiusY) during creation or the on fly.

earlygrey commented 3 years ago

Oh good point, it allows changing the estimated sides calculation on an existing ShapeDrawer instance which actually does add functionality.

Thanks for the work!