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

Allow flexible teleporter frames up to 10x10. #6938

Closed
wants to merge 3 commits into from

Conversation

martica
Copy link

@martica martica commented Jan 30, 2021

This change extends the frame detection of teleporters to allow frames up to 10x10, in all directions and orientations.

Additionally, it prevents teleporters with solid blocks inside the frame from working.

https://i.imgur.com/gkoKxQJ.jpg

The teleport block must be on a side, not a corner and all blocks inside the frame must not be solid, opaque or impaasible.
@SirEndii
Copy link
Contributor

And does the teleportation work?

@thiakil
Copy link
Member

thiakil commented Jan 30, 2021

Please switch to using our multiblock system rather than have it implement its own structure validation.

Sidenote: your usage of Optional seems quite redundant, null would probably suffice in many cases.

@pupnewfster
Copy link
Member

Also by solid blocks I presume things like rails still work fine inside of them?

@martica
Copy link
Author

martica commented Jan 30, 2021

Please switch to using our multiblock system rather than have it implement its own structure validation.

I'll check it out. I hadn't looked that far into the codebase. I was just working from what was already there.

And does the teleportation work?

Yep. For entities, mobs, players, players on horses, players in minecarts, minecarts alone. I don't think my changes affected that.

Also by solid blocks I presume things like rails still work fine inside of them?

Yes. It's possible I've made it too restrictive, the old code didn't care what was inside the frame at all. It may be better to allow users more flexibility and if there are blocks in the frame, don't worry about it. I'd still avoid allowing teleporter blocks/teleporter frame blocks inside as nested teleporters could lead to confusing behaviour.

Sidenote: your usage of Optional seems quite redundant, null would probably suffice in many cases.

Ah. My day job's code style is to generally avoid nulls.

@pupnewfster
Copy link
Member

I'll check it out. I hadn't looked that far into the codebase. I was just working from what was already there.

Ya we have a pretty decent/expansive system for multiblocks now in V10 (though I am not sure if some parts of it will need to be adjusted for purposes of the teleporter as it is 2D and in theory the frames can be shared). The main reason we didn't bother switching to it before when we wrote the new system and unified the rest of our multiblock formation code is it seemed a bit overkill for such a small and rigid structure as the teleporter used to be confined to.

I'd still avoid allowing teleporter blocks/teleporter frame blocks inside as nested teleporters could lead to confusing behaviour.

That is a good point and one thing I didn't realize would be an issue with this, though I think in theory it shouldn't be that hard in the multiblock validation to blacklist frames/teleporter blocks as invalid "inner" multiblocks.

…mes.

Outstanding issues:
- Teleporters work now, but at least in my save, existing ones need to be completely rebuilt.
- The frame blocks consistently trigger the multiblock validation, but the teleporter blocks themselves don't always trigger it...
- Shared frames do not work.
- Multiple teleporter blocks can be added to one frame.
- I needed to change Structure.java to allow for flat structures and I'm not sure what affect that will have on other uses of the class. It seems like it will just mean that precheck in CuboidStructureValidator will get called more often.
@martica
Copy link
Author

martica commented Jan 31, 2021

I got to the point where teleporters do something again. There are a bunch of rough edges still.

Copy link
Member

@pupnewfster pupnewfster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't review everything but left a couple comments. I also am not sure if the way this changes it breaks frames being able to be part of multiple teleporters (for example two both vertical ones that are perpendicular to each other). It may be worth making the frames structural and adjusting or just making another multiblock validator that isn't Cuboid not sure. A good part of me also hopes aidan will be able to way in here some as he knows the logic behind the multiblock system a lot better so may be able to better say when parts are breaking things entirely.

Comment on lines 18 to 19
public static final VoxelCuboid MIN_BOUNDS = new VoxelCuboid(1, 1, 1);
public static final VoxelCuboid MAX_BOUNDS = new VoxelCuboid(17, 18, 17);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these bounds are correct at all.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Those were just copied from elsewhere and adjusted while I tried to get it to work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I think a new validator that isn't directly a CuboidValidator may be better/more effective as realistically we want a VoxelPlane not a VoxelCuboid and there isn't a great way to specify the min bounds for a flat thing using the cuboid system.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I found the structure concept a bit hard to follow so I was trying to get to a working state again before I explored something more fit to purpose.

Comment on lines 152 to 153
if (minorPlane.hasFrame() && minorPlane.length() >= 2 && minorPlane.height() >= 2) {
// the plane has a frame and is at least two by two
if (minorPlane.hasFrame()) {
// the plane has a frame
Copy link
Member

@pupnewfster pupnewfster Jan 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change has some significant implications and in fact may entirely invalidate the point/efficiency of having two plane types (though @aidancbrady can probably say better).

Edit: Thinking about it further, this definitely probably should not be done, as a complete teleporter frame I believe should just be a "single" major plane if I remember how the planes system works (though I only have a rudimentary understanding of it).

*
* @return found cuboid, or null if it doesn't exist
*/
public static VoxelCuboid fetchCuboid(Structure structure, VoxelCuboid minBounds, VoxelCuboid maxBounds) {
public static VoxelCuboid fetchCuboid(Structure structure, VoxelCuboid minBounds, VoxelCuboid maxBounds, boolean filled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should not be used for this purpose as the entire point is for if it is all sides, the other method is much better used for cases like this as it was designed for non rectangular multiblocks and multiblocks that are not "full" for each face such as the thermal evaporation tower.

@@ -317,8 +318,14 @@ private MekanismBlockTypes() {
.withEnergyConfig(() -> TELEPORTER_USAGE, MekanismConfig.storage.teleporter)
.withSupportedUpgrades(EnumSet.of(Upgrade.ANCHOR))
.without(AttributeStateActive.class, AttributeStateFacing.class, AttributeParticleFX.class)
.with(new AttributeSecurity(), new AttributeMultiblock())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should rebase as I made changes earlier today to make it so that these attributes only ever have one instance created to save slightly on memory.

import java.util.Set;
import java.util.UUID;

import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use wildcard imports

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. IntelliJ kept doing that. :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the code formatting stylesheet we use with IntelliJ https://github.com/mekanism/Mekanism/blob/1.16.x/docs/GoogleStyle4Indents.xml

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will certainly help.

@martica
Copy link
Author

martica commented Jan 31, 2021

making the frames structural

Is there an example of this already? A regression in the shared teleporter frames would be ugly.

@pupnewfster
Copy link
Member

Structural glass is one, take a look at TileEntityStructuralMultiblock. I am not sure if there would be side effects such as it allowing using them in other multiblocks that are not the teleporter in place of structural glass. Though I think there may be a flag for what types of multiblocks structural ones are valid in as some only support reactor glass and not structural glass. In general I believe the validator also doesn't allow structural multiblocks as the frame but as we likely will need a custom validator that ends up consisting of a structure check of a single major plane we can have that validator allow it if necessary.

@martica
Copy link
Author

martica commented Jan 31, 2021

I've rolled by the WIP commit. If I get time to look at it again, I'll make another attempt.

@SimonLyke
Copy link

any updates on this?

@thiakil thiakil closed this Sep 21, 2023
@create-juicey-app
Copy link

Welp, we didn't got what we wanted, i dont know why

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.

6 participants