-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix #145 caused by #142 Make new feature opt-in #146
Conversation
Make new feature opt-in
I thought it was supposed to be like this. There apparently were also few more comments and disregarded stuff starting around #142 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as I do agree that, in most cases, it is better to keep original behavior as a default behavior.
Also voicing my opinion here about setting description, however don't really care enough for any of this to make any difference with approval:
For setting description (from PR #142) I do disagree with recommendations as those are opinion based rather than fact based:
This breaks expected behavior with digiline conducting tubes,
so it is recommended to enable this option
- unless you have specific builds that make use of the lack of vertical
- digiline connectivity and those are more important to you.
+ if you need direct vertical digiline connectivity for listed devices. Also
+ does not directly affect tubes despite explicitly mentioning those.
Which in turn is also kinda useless and maybe following would be better:
# If disabled, the devices will not be able to send or recieve digiline signals from the top
- # or bottom faces, regardless of the node rotation. This breaks expected behavior with digiline
- # conducting tubes, so it is recommended to enable this option unless you have specific builds
- # that make use of the lack of vertical digiline connectivity and those are more important to you.
+ # or bottom faces, regardless of the node rotation.
It is confusing that a toggle named "enable vertical" actually disables vertical tubes :p |
what about this ^ compromise? |
My opinion? Better. However about "This breaks expected behavior with digiline conducting tubes", is this something that could and should be fixed (a bug) or is it a feature that has to be paired with new setting? I mean if it actually is a bug then just cut/paste this line into a new issue. |
I consider it a bug. As I understand the goal of that change was to improve connectivity, but it obviously can't be solved with a general rule. There needs to be more granularity because what good is it that e.g. autocrafter can get signals from below, if the tubes can't deliver it. |
Merging as the comment in settingtypes.txt and the bugy behaviour is out of scope of this PR. |
I don't see why the new feature should force all existing servers to add a setting to opt-out when new servers can opt-in.
Edit: closes: #145