Fix redstone conduits and filters #881

2024-10-31T22:38:43Z

Metalit commented

2024-10-31 22:38:43 +00:00

(Migrated from github.com)

Description

Allows the yeta wrench to force connections for redstone conduits, and custom conduits if they choose to. Currently redstone conduits can even connect to air blocks, but I'm not sure if that should remain that way.

Also fixes some redstone filters:

NOT filter can be for both extract and insert instead of just insert
Sensor filter actually extracts analog signal instead of limiting the conduit to reading comparators
Timer filter reads its data correctly
Toggle filter reads its data correctly and doesn't write unnecessarily

Breaking Changes

The ConduitTicker interface has a new canHaveConnection method to determine if a forced connection is allowed. I'm not sure if this is actually breaking though, as it has a default that should effectively leave all existing implementations unchanged.

Checklist

My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you).
I have made corresponding changes to the documentation.
My changes are ready for review from a contributor.

# Description Allows the yeta wrench to force connections for redstone conduits, and custom conduits if they choose to. Currently redstone conduits can even connect to air blocks, but I'm not sure if that should remain that way. Also fixes some redstone filters: - NOT filter can be for both extract and insert instead of just insert - Sensor filter actually extracts analog signal instead of limiting the conduit to reading comparators - Timer filter reads its data correctly - Toggle filter reads its data correctly and doesn't write unnecessarily # Breaking Changes The ConduitTicker interface has a new `canHaveConnection` method to determine if a forced connection is allowed. I'm not sure if this is actually breaking though, as it has a default that should effectively leave all existing implementations unchanged. # Checklist - [x] My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you). - [x] I have made corresponding changes to the documentation. - [x] My changes are ready for review from a contributor.

Rover656 (Migrated from github.com) requested changes

2024-12-23 02:49:55 +00:00

Rover656 (Migrated from github.com) left a comment

These changes make sense from a read over - I'm not a fan of the conduit connection logic as a whole - but that's something I'm going to go and figure out later.

For now only a couple of comments, I'll try running this up to check it all works shortly.

Thanks for your help and apologies for the delay in a review!

These changes make sense from a read over - I'm not a fan of the conduit connection logic as a whole - but that's something I'm going to go and figure out later. For now only a couple of comments, I'll try running this up to check it all works shortly. Thanks for your help and apologies for the delay in a review!

src/api/java/com/enderio/api/conduit/ticker/ConduitTicker.java

					
				@ -22,8 +22,18 @@ public interface ConduitTicker<T extends ConduitData<T>> {

				    }

				    // TODO: I'd argue this goes into ConduitType, and then you can use getTicker() if you need additional context from it.

Rover656 (Migrated from github.com) commented

2024-12-23 02:47:11 +00:00

I don't think this method is adequately named - could you explain what the difference is from canConnectTo so we can figure a different name for this method? Also some javadoc may help :)

src/conduits/java/com/enderio/conduits/common/conduit/type/redstone/RedstoneConduitTicker.java

					
				@ -23,6 +23,7 @@ import java.util.Map;

				public class RedstoneConduitTicker implements IOAwareConduitTicker<RedstoneConduitData> {

Rover656 (Migrated from github.com) commented

2024-12-23 02:45:36 +00:00

I think this should check that the block isn't air :)

Metalit (Migrated from github.com) reviewed

2024-12-23 17:59:12 +00:00

src/api/java/com/enderio/api/conduit/ticker/ConduitTicker.java

					
				@ -22,8 +22,18 @@ public interface ConduitTicker<T extends ConduitData<T>> {

				    }

				    // TODO: I'd argue this goes into ConduitType, and then you can use getTicker() if you need additional context from it.

Metalit (Migrated from github.com) commented

2024-12-23 17:59:12 +00:00

Renamed it to canForceConnect with some javadoc. I don't really know what I was thinking with that name 😅

Renamed it to `canForceConnect` with some javadoc. I don't really know what I was thinking with that name 😅

Rover656 (Migrated from github.com) reviewed

2024-12-23 18:59:49 +00:00

src/api/java/com/enderio/api/conduit/ticker/ConduitTicker.java

					
				@ -22,8 +22,18 @@ public interface ConduitTicker<T extends ConduitData<T>> {

				    }

				    // TODO: I'd argue this goes into ConduitType, and then you can use getTicker() if you need additional context from it.

Rover656 (Migrated from github.com) commented

2024-12-23 18:59:49 +00:00

That makes more sense, thanks!

Rover656 (Migrated from github.com) reviewed

2024-12-27 19:03:10 +00:00

src/conduits/java/com/enderio/conduits/common/conduit/block/ConduitBlock.java

					
				@ -276,8 +262,8 @@ public class ConduitBlock extends Block implements EntityBlock, SimpleWaterlogge

				                }

				            } else {

Rover656 (Migrated from github.com) commented

2024-12-27 19:03:10 +00:00

To get this to work I also needed to change this to if (connectionState.isConnection()) {

If you'd agree with the change, please go ahead and make it and I'll merge the PR. Cheers!

To get this to work I also needed to change this to `if (connectionState.isConnection()) {` If you'd agree with the change, please go ahead and make it and I'll merge the PR. Cheers!