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

Re-adding working shapes plus some new ones #97

Merged
merged 13 commits into from
Apr 8, 2022
Merged

Conversation

Firigion
Copy link
Collaborator

@Firigion Firigion commented Jan 5, 2022

I re-introduced the old shapes implementations to ahve a version to ship with the next release until #94 is done. Also:

  • improved on the efficiency of cones and cylinders, which were awful (they used to check every block in the cuboid that inscribed them) and are now pretty decent. Only improvement that could be done is finding a better circle algorithm.
  • shpere and ellipsoid still do the awfull "check every block" thing, thos need fixing.
  • added working pyramid implementation off of the cone code, making hollow pyramids work was not trivial
  • since I had a "pyramid with arbitrary base" thing going, I added polygon and star-based pyramids. The hollow forms of these were less trivial and don't fully work (polygons are ugly anyway). A corresponding warning message is sent to the player, recommending the use solid shapes with a hollow wand if they don't get the expected results.
  • added an interactive clear button in the brushes list. I didn't mean to do it all in one PR, but I found the issue while doing this, sorry.
  • added the new things to the documentation, and fixed a few things in CONTRIBUTING.md while at it.

Should we add a link to the discor dserver/channel in either readme, the docs or the contribution manual?

se.sc Outdated Show resolved Hide resolved
se.sc Outdated Show resolved Hide resolved
@Ghoulboy78
Copy link
Owner

Wait, I see you've added a bunch of new functions. Do you intend on keeping them even after I implement the new shapes from shapes.scl, or not? cos if so it's best to mention that you're gonna remove them, and if not you could add comments explaining what they do.

@Firigion
Copy link
Collaborator Author

Firigion commented Jan 5, 2022

I'll add comments. Some functions are staying, since they are used in shapes that are not gonne be replaced. And I'd discuss the replacement of some of the rhapes anyway, I wrote some decent code this time. We'll have to test efficiencies once the library is finished.

Right now, diamond can't get much better, I'm pretty sure, I do absolutely no extra check, just calculate the blocks needed in the most straightforward way, i even minimized the ammount of divisions needed.

Pyramid is quite good too, for the same reason. Cone and cylinder could use a better circle algorithm, right now for a circle with radius r I'm doing r²/2 checks (the naive approach is 4r²), but that's still O(r²). Note that the area in a disc scales with r², so it's not such a bad algorithm, and the circle is calculated in the same pass as the disc, so that's not an overhead.

Only case where an improvement would actually impact the performance is in hollow cone, since making the case where height is smaller than radius work properly requires checking all the r²/2 blocks for every layer (r being smaller each layer). We can improve this a bit in detriment of the shape of the cone, which I think is quite good now.

Shperes and eliposids I didn't touch and those are really bad. They check 8r³ blocks one by one.

@Firigion Firigion linked an issue Jan 7, 2022 that may be closed by this pull request
6 tasks
@Firigion Firigion added bugfix Bug fix enhancement New feature or request labels Jan 8, 2022
@Ghoulboy78 Ghoulboy78 requested a review from altrisi March 25, 2022 10:28
@Ghoulboy78 Ghoulboy78 merged commit 15c0d4a into master Apr 8, 2022
@Ghoulboy78 Ghoulboy78 deleted the working-shapes branch April 8, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fix enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some shapes are broken after merge of #49
2 participants