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

GT6 Styled Pipes & Cables #1264

Merged
merged 15 commits into from
Nov 25, 2017
Merged

Conversation

Antifluxfield
Copy link
Contributor

@Antifluxfield Antifluxfield commented Oct 30, 2017

  • Pipes won't auto matically connect to each other, unless you directly place them on the block they could connect to. Otherwise you should use wrenches to connect or disconnect them. Also, for fluid pipes, you can disable its input at specific side by sneak-rightclick with your wrench, and click again to re-enable them. These feature changes can be disabled by config.
  • Cables connections can also work simillar as pipes, but this is disabled by default. If you enable the config, you need to use wire cutters or soldering irons to connect/disconnect the cables.
  • Based on the cable connection change above, you can enable another feature which requires soldering materials when connecting wires.
  • Add Quadruple and Nonuple fluid pipes.
  • ItemPipe tick time customization #1255 is also included here, so I closed that pr.

@draknyte1
Copy link
Collaborator

So are these meant to immitate GT6 pipes?
I think the cables were fine as they were and didnt need changing..

@Antifluxfield
Copy link
Contributor Author

Yes, just immitate.
So maybe I should set another individual config to disable the changes for cables?

@draknyte1
Copy link
Collaborator

draknyte1 commented Oct 30, 2017

Nobody had an issue with cables at all, just the useless pipes.

Id say cable changes should default off, and continue to function exactly the same unless someone wants otherwise via config.

Edit: Does this change sloshing in pipes? Thats basically half the issue, the other half being people didnt like automatic connections.

Did you do this from scratch or utilise GT6 src? Good work either way.

@Antifluxfield
Copy link
Contributor Author

Antifluxfield commented Oct 30, 2017

Thanks.
I did took a look at GT6 src for a while, but soon found it hard to peel out the code I need, so I decided to write it myself 😝
Anyway, I think this pr still needs more tests to make sure all of the changes are working properly.

@draknyte1
Copy link
Collaborator

I figured doing it from scratch was easier than merging in 6 code. But all the non-coders were hell bent on us using code from 6.

Im sure this PR will sit open for a while, both for test results and code review.

@Dream-Master
Copy link

@Antifluxfield If this pull is ready please do one for gtnh too. Thanks

@MauveCloud
Copy link
Collaborator

Edit: Does this change sloshing in pipes? Thats basically half the issue, the other half being people didnt like automatic connections.

I think this part will help with that:

Also, for fluid pipes, you can disable its input at specific side by sneak-rightclick with your wrench

If I'm understanding that correctly, it would make the pipe side effectively a one-way valve. Personally, I'm still not convinced sloshing is as big a deal as others make it out to be. Seems to me that simply keeping the pipes full (or nearly) should avoid most sloshing issues.

@netmc
Copy link

netmc commented Oct 30, 2017

I believe the sloshing fix that Greg implemented in GT6 was to make sure that fluid when it enters a pipe cannot exit out the same direction it entered. It will try the other available directions first. This allows a tiny bit of fluid in a pipe to eventually drain to one end.

@MauveCloud
Copy link
Collaborator

This allows a tiny bit of fluid in a pipe to eventually drain to one end.

So this is for the situtation where you've stopped using one fluid and want to switch to something else? I can believe there are some fluids valuable enough to not want to lose even trace amounts by breaking and replacing the pipes, but it seems like those would also be valuable enough to justify adding pump covers and/or shutter modules to do this.

@netmc
Copy link

netmc commented Oct 30, 2017

That is indeed the situation. It is mainly a quality of life thing as if you have a very long line of pipes, and needed to switch from one fluid to another, simply placing a tank on the pipe wouldn't drain it completely. You would need to break and replace every single pipe in the line to make it happen. If you had 100 pipes in a hard to reach area, it would be a royal pain to do so.

@OvermindDL1
Copy link

Specifically GT6 pipes remember from what directions a fluid last flowed 'into' it and will only output to other faces after that (resetting that list, it is only persisted for one tick). That does indeed allow fluids to eventually 'flow out' as long as some pipe outputs to something that can not input back to it.

@Antifluxfield
Copy link
Contributor Author

Antifluxfield commented Oct 31, 2017

GT6 pipes remember from what directions a fluid last flowed 'into' it and will only output to other faces after that

Well, we do have that record, and I've just noticed that we weren't actually using it... It's quite easy to fix so.
So what's your idea about the input restriction? Shall we keep that function, make it more expensive(for example, consumes rubber rings to stuck the input), or just cancel it?

@Antifluxfield
Copy link
Contributor Author

Antifluxfield commented Oct 31, 2017

@MauveCloud I mean I forgot to use it...

@MauveCloud
Copy link
Collaborator

I figure there must be some additional benefit(s) to having the pipes not automatically connect (otherwise, why would Greg set up GT6 pipes to behave that way?), but I agree that having 2 options for the pipes (one to allow toggling connections via wrench and one to allow toggling inputs via sneak-click with wrench) instead of just one would be a good idea.

@draknyte1
Copy link
Collaborator

Just because Mr Gregorious does something doesn't mean it's the best way.
I am strictly talking about a config to enable the original (current) behaviour and the new non-auto connection behaviour. I am content with pipe connections as they are and would like this behaviour to still be available.

@Antifluxfield
Copy link
Contributor Author

@draknyte1 There already is a config to disable the non-auto connection...
@MauveCloud For that hitbox issue, try with the latest commit. I think this should be acceptable for everyone.
@Blood-Asp Yes, it is ready for review.

@MauveCloud
Copy link
Collaborator

For that hitbox issue, try with the latest commit.

Seems to work okay for me, though I notice an amusing side effect (at least in SSP, idk if it would also happen in SMP): while holding a wrench or cover (or presumably one of the other related items), I can walk on uncovered small pipes as if they're full blocks, but when I switch to something else, I fall a fraction of a block.

Unlike everyone else seems to imply they do, I don't make mistakes hooking up pipes, so it's never been an issue, only sloshing has been.

TBH, I haven't noticed much issue with sloshing myself, but I decided to playtest this PR a bit anyway (though I'm rather slow at that, perhaps partly due to doing so in a new SSP game). Looking at #1253 again, I'm not sure it's so much a question of "mistakes" hooking up pipes, but concerns over the cost of cover materials:

yes, the major problem everyone's been having is that the pipes autoconnect forcing you to waste materials on covers

I have to wonder whether this commenter is aware that wood plank covers can be made at a rate of 2 per wood slab, which seems pretty cheap to me, considering how renewable wood is. Technically, painting the pipes different colors will already prevent fluids from transferring between them (if I'm reading the code correctly), but I don't think it will prevent them from looking connected (though I think this PR might fix that even with the option set to allow automatic pipe connections), and afaik they can only be painted after being placed, so one would need to temporarily add some covers or paint the pipes while they're empty.

@MauveCloud
Copy link
Collaborator

MauveCloud commented Nov 8, 2017

Update: I just discovered a much more serious issue. With GT6StyledPipesConnection=true in GregTech.cfg, pipes can lose their connections somewhere in chunk loading/unloading. I went away for a bit, and came back to find my steam machines were no longer connected to the pipe behind them. I hadn't realized when I was setting them up that the pipe was in a different chunk than the machines, but I should only have to worry about chunk boundaries when setting up multiblocks, not when setting up early-game single-block steam machines and pipes. Here's a screenshot with the pipes re-connected but with NEI chunk boundaries showing:
2017-11-07_20 41 09

Edit: side note:

There already is a config to disable the non-auto connection...

True, but as I understand it turning that option off also prevents one from sneak clicking with a wrench to disable input from a given pipe face (i.e. making it a one-way valve). Without that, what does this PR actually do to prevent/mitigate sloshing, as draknyte1 seems to want? I'm a little vague on that detail.

@MauveCloud
Copy link
Collaborator

Latest commit to this PR seems to have fixed the chunkloading issue. Now I'm curious how faithful you're trying to be to the pipes and cables being "GT6 Styled". I just did a bit of testing in creative mode with a recent build of GT6, and noticed a couple of things that haven't been carried over:

  1. GT6 shows diagonal lines to form sort of an "X" shape in certain grid sections to show me ahead of time which faces I'll be disconnecting with the wrench/cutter. The chat messages from this PR tell me afterward whether I've connected or disconnected the pipe, and with tiny to medium pipes, it's usually easy to see directly which faces are connected, but when I get to large or huge pipes (and 12x cables/16x wires) it gets a little harder.
  2. GT6 allows pipes/cables to be extended towards empty air, and stay extended even if a solid block like sand (which a pipe/cable can't really interact meaningfully with) is placed against that face.

@MauveCloud
Copy link
Collaborator

Draw the cross in the hint grid

Sorry, perhaps I wasn't clear enough in my description of what GT6 does. If you're going to bother with this, I'd prefer you do it right. GT6 shows the Xes based on which sides are connected or not, so I can tell ahead of time whether I'll be connecting or disconnecting the pipe (or cable):
2017-11-09_09 25 41
What you've done is show an X for whichever section I'm aiming at, regardless of which faces are currently connected:
2017-11-09_09 26 26
and that is considerably less useful (unless maybe I had the crosshair turned off somehow).

@Antifluxfield
Copy link
Contributor Author

Ummmm...

@netmc
Copy link

netmc commented Nov 10, 2017

I do like having a highlight to know which control is active, but the X's show the current pipe connections. This is most useful when connecting huge pipe since the pipe fills the entire block, there is no watt to know which faces ate open and connecting to adjacent pipe.

@Antifluxfield Antifluxfield force-pushed the GT6-styled_pipes branch 2 times, most recently from 0640ff3 to f8a18c8 Compare November 13, 2017 02:29
@Blood-Asp Blood-Asp merged commit 7cc77f5 into Blood-Asp:unstable Nov 25, 2017
@Blood-Asp
Copy link
Owner

I get spammed by lots fo GL Errors when pointing the wrench on a pipe:

[20:58:12] [Client thread/ERROR]: ########## GL ERROR ##########
[20:58:12] [Client thread/ERROR]: @ Post render
[20:58:12] [Client thread/ERROR]: 1282: Invalid operation

@draknyte1
Copy link
Collaborator

This is why you don't merge untested bs unreviewed PRs...

@Blood-Asp
Copy link
Owner

Oh yes, because you would have seen it with a single look at the code and i never check anything...

I make testbuilds for everyone to test them in real enviroments because that works in reality compared to your holy reviews...

@MauveCloud
Copy link
Collaborator

I'll admit I hadn't noticed it either, mostly because I had been leaving the log window closed while playing (plus I'd been taking a break from even playing Minecraft), but after adjusting connections, the GL Error happens to me with the custom build I made as well, so I have to share draknyte1's skepticism about the claimed pre-merge testing.
There are two additional reasons I was surprised this was suddenly merged:

  1. It was still marked as "in progress", though with no further commits in almost 2 weeks, perhaps somebody should have asked whether it was considered complete now.
  2. None of the 4 people reviews were requested from had approved it. (though perhaps they were waiting until it was no longer "in progress" to start reviewing it)

@draknyte1
Copy link
Collaborator

draknyte1 commented Nov 26, 2017

Typical @Blood-Asp tier testing.
There was a build with this PR compiled available to test (Also built in is #1247) available within the Dev Discord, so not testing it pre-merge for more than a second isn't really an excuse..

@Blood-Asp
Copy link
Owner

I never said i did not try a test version before merging. In fact i did. I tested the stated changes and saw them working. Also where there multiple persons that tried the PR before and did not notice the GL Error.

As alternative i could have waited another few weeks and maybe we had got some reviews, likely not all anyways. And there where enough PRs before that had all Reviewers Appove of them and still errors.

@mitchej123
Copy link
Contributor

mitchej123 commented Nov 27, 2017

I've been playing with this recently noticed that even if an item pipe shows unconnected visibly (specifically noticed with machines), it will still route into the machine.

EDIT: Should have clarified, it's downstream on a bleeding edge version of the GTNH fork; going with the assumption it impacts upstream as well.

@NNM42
Copy link

NNM42 commented Nov 27, 2017

U say about this? (05.09.30)
2017-11-27_18 35 47

@Antifluxfield Antifluxfield deleted the GT6-styled_pipes branch November 29, 2017 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants