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

Fix #145 caused by #142 Make new feature opt-in #146

Merged
merged 3 commits into from
Dec 17, 2024
Merged

Conversation

SwissalpS
Copy link
Contributor

@SwissalpS SwissalpS commented Dec 17, 2024

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

@S-S-X
Copy link
Member

S-S-X commented Dec 17, 2024

I thought it was supposed to be like this.

There apparently were also few more comments and disregarded stuff starting around #142 (comment).
Could consider those while at it, not sure if it's important. Probably not that important but more about "maybe nice to have" stuff.

Copy link
Member

@S-S-X S-S-X left a 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.

@SwissalpS
Copy link
Contributor Author

It is confusing that a toggle named "enable vertical" actually disables vertical tubes :p

@SwissalpS
Copy link
Contributor Author

what about this ^ compromise?

@S-S-X
Copy link
Member

S-S-X commented Dec 17, 2024

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.

@SwissalpS
Copy link
Contributor Author

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.

@SwissalpS
Copy link
Contributor Author

Merging as the comment in settingtypes.txt and the bugy behaviour is out of scope of this PR.

@SwissalpS SwissalpS merged commit 858154c into master Dec 17, 2024
2 checks passed
@SwissalpS SwissalpS deleted the SwissalpS-patch-1 branch December 17, 2024 19:43
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.

Vertical digiline tubes no longer work as they did
2 participants