[enhancement] better redstone dust placement
lublak opened this issue ยท 5 comments
I will try to implement it myself and make a pull request.
You can do it too, of course, but I like to help a bit, if I can, and not just throw my ideas in here.
blue: player position (the player position is only important if only one connection is available)
black: cell
red (in black): a possible connection point, repeater, other redstone line (only if the side is enabled), block?, comparator, etc.
red (in white): the new redstone dust to be placed with the connections
@dannydjdk what do you think of that?
So basically:
If only one connection exists in the adjacent fields:
Connect one connection and then, depending on the player orientation, the other.
Otherwise:
Connect only all available connections.
Interesting... so it would behave more like vanilla redstone, but allow the player to override. I do find that the current placement is a bit cumbersome. Sometimes you have to click multiple times to get the connections you want. I'm a little concerned that if we complicate it too much without fully replicating vanilla, we might confuse players. On the other hand, your method seems pretty intuitive, possibly more so than vanilla.
Here are my thoughts on implementation:
The PanelCellNeighbor class gets passed to cells when a neighbor updates its state. It has a method called canConnectRedstone which is very similar to the vanilla BlockState method of the same name. In vanilla, this tells you whether or not "passing" redstone will visually connect to that side of the block. In PanelCellNeighbor, for blocks on the edge of the panel, it just passes along the result from the BlockState. For neighboring cells, it passes a hard-coded result.
Currently, this method is just used by redstone dust to determine whether or not it should pick up weak redstone signals. (If you're not entirely clear on the distinction between weak and strong power, this might help.)
Therefore, I think the best approach is to add canConnectRedstone(PanelCellSide) to the IPanelCell interface and implement it in all IPanelCell classes. PanelCellNeighbor would then pass on that result instead of the hard-coded result. Then, when placing redstone, it will connect where that method returns true based on the rules above.
Does that make sense?
On a related note (but perhaps a discussion for a new issue), one of the problems with the current redstone is that it might be confusing to know where to click and what's going to happen when you click within the redstone cells. It might be useful to have a ghost preview there too.
Yes it is similar to Minecraft redstonee.
I have thought about how to combine your logic of redstone dust with that of vanilla.
I think a bit of vanilla may be deviated and have tried to combine both advantages.
It would also be possible to have two different dust types. Vanilla Redstone dust or then just Redstone dust, which behaves normal and then a Super Redstone dust.
My opinion is rather negative to the two dust types because it is only complicated.
The implementation for this would not be necessary at the state update but only at the placement.
Maybe an onBeforePlace(PanelCellNeighbor frontNeighbor, PanelCellNeighbor rightNeighbor, PanelCellNeighbor backNeighbor, PanelCellNeighbor leftNeighbor) method.
This is called before the component is placed. In most cases it will be empty but in the redstone dust it will be something like this:
frontEnabled = frontNeighbor.canConnectRedstone();
rightEnabled = rightNeighbor.canConnectRedstone();
backEnabled = backNeighbor.canConnectRedstone();
leftEnabled = leftNeighbor.canConnectRedstone();
/*
if only one connection is avaible or none
build with a Karnaugh map, can be optimized I think
and also just no warranty on it
was just on the quick
edit: optimized
*/
if(!(
(frontEnabled || rightEnabled || backEnabled)
&& (frontEnabled || rightEnabled || leftEnabled)
&& (frontEnabled || backEnabled || leftEnabled)
&& (rightEnabled || backEnabled || leftEnabled)
)) {
if(frontEnabled) {
backEnabled = true;
} else if(backEnabled) {
frontEnabled = true;
} else if(rightEnabled || leftEnabled) {
backEnabled = true;
} else {
frontEnabled = true;
backEnabled = true;
}
}
I would prefer your current implementation but the "placing" of the redstone should be a bit more logical.
The "disconnect" or the "connect" of the redstone dust I find great even if it is not quite vanilla. But so is e.g. the Torch.
something like "canConnectRedstone" was also my idea because you can use this then also simply for possibly further components or addons easier.
A ghost preview is also a good idea.
Something like: segmentClicked -> segmentHovered
This makes sense. I was thinking of using the neighborChanged method because it does called when the component is placed, but there is no way to know within the method that it is being called on placement vs some other neighbor change.
I can imagine a lot of scenarios where having an onPlace method would be helpful. onBeforePlace could be useful if we can imagine a scenario where it would cancel placement after looking at its potential neighbors. The convention with other IPanelCell methods has been that it returns a boolean indicating whether its neighbors should be notified of an output change. We could break that convention here and have it return whether or not it finds the neighborhood suitable for placement and then just always notifying neighbors. The overhead would be negligible, especially since it only happens on player interaction. I'm kind of thinking out loud here, but I'm imagining a future implementation of a component that only wants to be placed on the edge of the tile or only next to a particular cell.
That's probably more geeky rambling than was necessary here, so in short... I agree. ๐