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

Updating shapes to use shapes.scl and fix some minor things #94

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Ghoulboy78
Copy link
Owner

@Ghoulboy78 Ghoulboy78 commented Dec 31, 2021

It replaces cuboid, cylinder, sphere, cone and pyramid with shapes.scl equivalents.
Obviously it will work better when gnembon/fabric-carpet#1154 is accepted, and cuboid does actually need to be reverted and the old code fixed.

@Ghoulboy78 Ghoulboy78 changed the title Updating shapes tl uj Updating shapes to use shapes.scl and fix some minor things Dec 31, 2021
@Ghoulboy78 Ghoulboy78 linked an issue Dec 31, 2021 that may be closed by this pull request
6 tasks
@Ghoulboy78 Ghoulboy78 marked this pull request as draft December 31, 2021 14:14
@Ghoulboy78 Ghoulboy78 marked this pull request as ready for review January 1, 2022 16:42
Copy link
Collaborator

@Firigion Firigion left a comment

Choose a reason for hiding this comment

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

Is this working off of your suggested changes to shapes.scl in gnembon/fabric-carpet#1154? Because right now it doesn't work with the current shapes.scl. I'd also like to know it that PR depends on gnembon/fabric-carpet#1218, because as discusse on Discord, it doesn't look like perople are gonna agree on implementing your suggested changes...

Edit: I reviewed the linked PR for shapes.scl, needs some changes before it's ready. If you confirm this PR here is working off of that one, i'll intall the suggested library and test this PR again.

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

Is this working off of your suggested changes to shapes.scl in gnembon/fabric-carpet#1154? Because right now it doesn't work with the current shapes.scl. I'd also like to know it that PR depends on gnembon/fabric-carpet#1218, because as discusse on Discord, it doesn't look like perople are gonna agree on implementing your suggested changes...

This is indeed working off of my changes to shapes.scl in gnembon/fabric-carpet#1154, as mentioned in the title, but gnembon/fabric-carpet#1218 has got nothing to do with this, because gnembon/fabric-carpet#1154 uses a workaround so I don't have to use it.
I will merge this pr after gnembon/fabric-carpet#1154 gets merged and sent into a release, so you have time to review, especilly cos now I0ve got a lot more work to do on that one.

@altrisi
Copy link
Collaborator

altrisi commented Jan 2, 2022

One thing that could be done is adding the newer Carpet version in a requires block too, so we make sure things will work correctly.

@Ghoulboy78
Copy link
Owner Author

I'm afraid I don't really know how 'requires' works, so I couldn't do it myself...

@Firigion Firigion removed a link to an issue Jan 7, 2022
6 tasks
@Ghoulboy78 Ghoulboy78 added the requires carpet pr Waiting on a pr in Fabric Carpet label Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires carpet pr Waiting on a pr in Fabric Carpet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shapes should be replaced with more efficient versions used in carpet /draw
3 participants