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

Merge further improvements submitted to my fork #15

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

Conversation

h-v-smacker
Copy link
Contributor

fluxionary greatly improved the code of the mod recently, so I'm propagating these changes to upstream. A lot of redundancy in definitions has been removed overall.

@TumeniNodes
Copy link
Owner

TumeniNodes commented Feb 27, 2020

There's just no way I can accept this PR.
Just testing now and right off the bat, the groups are broken for every facade node, which renders them unbreakable.

I'm not gonna go through each of these changes to check for further issues.
These should have been thoroughly tested individually... to throw a PR at me this size and with so many various changes, is not something I'm going to even consider.

There could be numerous problems in all this mess, and tbh I do not have the time to go through it all one by one.

@h-v-smacker
Copy link
Contributor Author

I'll look into what happens with groups (I tested on creative and on v4 back in October, which obviously could hide the issue you're seeing today), but otherwise most changes in the PR must be purely cosmetic. Indentation and such.

@TumeniNodes
Copy link
Owner

Just to make you aware, I have been getting ready to offer some of my mods (this one included) over to minetest_mods, because I have had less and less time, to fiddle these days.
Quite a few PRs and Issues came along which I did not even take notice of...
it doesn't help that it seems while MS is trying to push it's horrible changes to notifications, they are either intentionally making the old system it suck so people want the new one, or they are too busy and forgot about it.
So all I ever see is the last date I commented on just about everything (even with my acct set up to send notifications to my phone/email etc.)
And I opted out of the horror of the beta mess.

So anything being presented currently will have to remain on hold until I get these over there.
Then I will feel much better knowing people are not waiting around forever just to hear back, because i know that can get annoying and frustrating.

So I'm not leaving you or anyone else hanging.. I just need to get these things somewhere they can get better attention

Thank you

@@ -0,0 +1,2 @@
facade.in_creative_inventory (Facades show in inventory) bool true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
facade.in_creative_inventory (Facades show in inventory) bool true
facade.in_creative_inventory (Enable facades in creative inventory) bool true

@@ -0,0 +1,2 @@
facade.in_creative_inventory (Facades show in inventory) bool true
facade.protect_inventories (Facade shaper is protected) bool false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
facade.protect_inventories (Facade shaper is protected) bool false
facade.protect_inventories (Enable shaper protection) bool false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants