Skip to content

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.

@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...

@Ghoulboy78 Ghoulboy78 dismissed Firigion’s stale review January 3, 2022 11:08

Resolved issues

@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