fix: regenerate addressess and update merge state when gate position changes #310

sentialx · 2021-05-17T10:05:50Z

This PR should fix issues when moving the gate by using mods like WarpDrive, Valkyrien etc.

Nine chevron address stays the same when the gate moves and seven chevron address regenerates.

Fixes #253

…changes

MrJake222 · 2021-05-17T10:18:19Z

Won't it spam StargateNetwork with old addresses not pointing to valid gates? You don't seem to remove old addresses from the mapping.

You can query those with /sgquery command.

sentialx · 2021-05-17T11:09:52Z

My generateAddresses function calls the setGateAddress function which seems to remove the old address:
https://github.com/sentialx/AUNIS/blob/c45f70404750b76da843888da99235b5db017c59/src/main/java/mrjake/aunis/tileentity/stargate/StargateAbstractBaseTile.java#L422

MrJake222 · 2021-05-17T14:11:18Z

Okay, didn't remember that one

MrJake222

You probably shouldn't do sth like this. Find the cause that's passing null there. Or at least handle it higher.

slava110 · 2021-05-17T19:30:44Z

src/main/java/mrjake/aunis/tileentity/stargate/StargateClassicBaseTile.java


 	@Override
 	public void update() {
 		super.update();

 		if (!world.isRemote) {
+			if (!lastPos.equals(pos)) {
+				lastPos = pos;
+				generateAddresses(!hasUpgrade(StargateClassicBaseTile.StargateUpgradeEnum.CHEVRON_UPGRADE));


So if there's no chevron upgrade stargate will reset address after chunk will be unloaded and loaded again? Because lastPos field will be null. Custom stargate address will not work without chevron upgrade

What would you recommend to check whether the tile has moved?

Btw, does it actually matter? The address will stay the same, since the generator is using the position as seed.

Slava mentions custom Stargate addresses, so someone using commands won't be happy with those changes.

Maybe save a boolean to NBT like addressOverriddenByCommand or just fixedAddress with a docstring indicating it handles address persistence when used with commands?

Ah, I missed that command. I've added lastPos assignment in onLoad, and I think it works now.

Did you check that? With command and with Warp drive or other gate-moving mods?

Not sure about that. I think now code inside !lastPos.equals(pos) condition will not be called at all. Because position will be set in onLoad method after TileEntity creation and it's not possible that this position will not be equal to the current gate position

That's what I'm thinking. onLoad executes before update and whole effort of making this mod WarpDrive compatible is worthless.

slava110 · 2021-05-17T20:03:19Z

src/main/java/mrjake/aunis/tileentity/stargate/StargateUniverseBaseTile.java

@@ -42,105 +42,103 @@

 	// --------------------------------------------------------------------------------
 	// Dialing
-


It's recommended to do code optimisations in different commits/PRs. Because it's a bit harder to review PR with this

I tried to keep these files as they were, but IntelliJ doesn't have an option to disable formatting on save, sorry for that.

Yes it does. If you're making commits via Intellij Idea there is checkmark Do you want to optimise code before commit?. And there's no automatic optimisations on save unless you turned this on by yourself. Check IDE settings and look at this checkmark on commit

I'm not doing commits via intellij, but it's formatting on save, which doesn't seem to have an option to disable that.

I've read there is a macro bound to Ctrl+S by default in intellij

slava110 · 2021-05-17T20:26:57Z

src/main/java/mrjake/aunis/tileentity/stargate/StargateAbstractBaseTile.java

@@ -1043,6 +1038,15 @@ public final void updateMergeState(boolean shouldBeMerged, EnumFacing facing) {
 		else {
 			onGateMerged();
 		}
+
+		// If the gate has already been merged, there is no need to merge it again.
+		// However, when the gate position changes, the members base pos should be updated as well.


when the gate position changes
But it will be updated every time when updateMergeState will be called with (shouldBeMerged && this.isMerged) == true. Should be fine anyway? :I
Also you can move this stuff higher probably (above if (!shouldBeMerged) condition, line 1029)

But it will be updated every time when updateMergeState will be called with (shouldBeMerged && this.isMerged) == true. Should be fine anyway? :I

If that's the case, this means that the gate changed its position. There is no other code path that could trigger the if statement.

slava110 · 2021-05-17T20:28:05Z

src/main/java/mrjake/aunis/tileentity/stargate/StargateAbstractBaseTile.java

+				address.generate(random);
+			}
+
+			this.setGateAddress(symbolType, address);


Address might be null

Also there is so much code stripped of this method compared to the original code from onLoad

Yeah beacause it was basically setGateAddress repeated.

HaleonGoemea · 2021-05-26T11:32:26Z

I would be willing to test, but idk how to turn this into a .class file. I have figured out that's what I need to do, but it always errors.

I apologize if it's rude to comment here, and will delete if so

MrJake222 · 2021-05-26T11:56:18Z

I would be willing to test, but idk how to turn this into a .class file. I have figured out that's what I need to do, but it always errors.

I apologize if it's rude to comment here, and will delete if so

Run ./gradlew build and search in build/libs for a .jar file.

sentialx added 2 commits May 17, 2021 12:01

fix: regenerate addressess and update merge state when gate position …

6f12ebe

…changes

fix: spaces -> tabs

c45f704

sentialx force-pushed the address-regen branch from ade8534 to c45f704 Compare May 17, 2021 10:13

sentialx added 3 commits May 17, 2021 21:05

fix: add onGateMergeRequested

2882031

fix: updateMembersBasePos

56ad3c2

fix: null checks

c8d7675

MrJake222 reviewed May 17, 2021

View reviewed changes

slava110 suggested changes May 17, 2021

View reviewed changes

sentialx added 2 commits May 17, 2021 23:49

fix: apply suggestions

b8ad49a

fix: remove unnecessary changes

44bc860

sentialx force-pushed the address-regen branch from b22a078 to 44bc860 Compare May 17, 2021 22:01

sentialx requested review from MrJake222 and slava110 May 17, 2021 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: regenerate addressess and update merge state when gate position changes #310

fix: regenerate addressess and update merge state when gate position changes #310

sentialx commented May 17, 2021

MrJake222 commented May 17, 2021

sentialx commented May 17, 2021 •

edited

Loading

MrJake222 commented May 17, 2021

MrJake222 left a comment

slava110 May 17, 2021

sentialx May 17, 2021

sentialx May 17, 2021

MrJake222 May 17, 2021

MrJake222 May 17, 2021

sentialx May 17, 2021

MrJake222 May 18, 2021

sentialx May 18, 2021

slava110 May 18, 2021

MrJake222 May 18, 2021

slava110 May 17, 2021

sentialx May 17, 2021

slava110 May 18, 2021

sentialx May 18, 2021

MrJake222 May 18, 2021

slava110 May 17, 2021

sentialx May 17, 2021 •

edited

Loading

slava110 May 17, 2021

MrJake222 May 17, 2021

sentialx May 17, 2021

HaleonGoemea commented May 26, 2021

MrJake222 commented May 26, 2021

		@@ -42,105 +42,103 @@

		// --------------------------------------------------------------------------------
		// Dialing

fix: regenerate addressess and update merge state when gate position changes #310

Are you sure you want to change the base?

fix: regenerate addressess and update merge state when gate position changes #310

Conversation

sentialx commented May 17, 2021

MrJake222 commented May 17, 2021

sentialx commented May 17, 2021 • edited Loading

MrJake222 commented May 17, 2021

MrJake222 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentialx May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaleonGoemea commented May 26, 2021

MrJake222 commented May 26, 2021

sentialx commented May 17, 2021 •

edited

Loading

sentialx May 17, 2021 •

edited

Loading