-
Notifications
You must be signed in to change notification settings - Fork 199
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
1.19 update #638
base: master
Are you sure you want to change the base?
1.19 update #638
Conversation
1.19 update fixes.
Thank you for submitting! I'll need a bit to go through all of the changes, as said on discord. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I still need to test it, but it looks very promising. Could you check my feedback? It's mainly a few small things
.github/workflows/dotnetcore.yml
Outdated
@@ -4,6 +4,7 @@ on: | |||
push: | |||
branches: | |||
- master | |||
- 1.19-update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/MiNET/MiNET.Console/log4net.xml
Outdated
@@ -107,7 +107,7 @@ | |||
</appender> | |||
|
|||
<root> | |||
<level value="INFO" /> | |||
<level value="TRACE" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put the default back to INFO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// TODO - 1.19-update | ||
|
||
//foreach (KeyValuePair<string, int> keyValuePair in ItemFactory.IdToRuntimeId) | ||
//{ | ||
// if(keyValuePair.Key.Equals("sapling")) Console.WriteLine(keyValuePair.Key); | ||
//} | ||
//var itemId = ItemFactory.GetItemIdByName("minecraft:sapling"); | ||
//Assert.AreEqual(6, itemId); | ||
|
||
//Item item = ItemFactory.GetItem("minecraft:sapling"); | ||
//Assert.AreEqual("minecraft:sapling", item.Name); | ||
//Assert.IsNotNull(item as ItemBlock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix this? Could change it into ID based
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
speaking in general, as a testing automation engineer in the past, I can say that the MINET auto tests look bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I'm done with the features, I could write some good tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the current tests are... Not great, i agree with that. It would be nice to get some proper ones at some point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it so that it would at least test something.
@@ -189,7 +189,9 @@ public ChunkColumn GetChunk(ChunkCoordinates coordinates, IWorldGenerator genera | |||
if (flatDataBytes != null) | |||
{ | |||
Buffer.BlockCopy(flatDataBytes.AsSpan().Slice(0, 512).ToArray(), 0, chunkColumn.height, 0, 512); | |||
chunkColumn.biomeId = flatDataBytes.AsSpan().Slice(512, 256).ToArray(); | |||
|
|||
// TODO - 1.20 - update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you could look into loading the biome id's in still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be supported, since the client is currently working with 3d chunks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be active?
// TODO - 1.20 - update | ||
//for (int i = 0; i < chunk.biomeId.Length; i++) | ||
//{ | ||
// chunk.biomeId[i] = biomeId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you could keep this working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? But before that, I need to rework data storage in chunks, since the current one is outdated and inconvenient.
When I'm done, it will turn into something like:
subChunk.Biomes.Fill(id);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be active?
1.19 update fixes.
@@ -1189,7 +1317,7 @@ public virtual void Teleport(PlayerLocation newPosition) | |||
Monitor.Exit(_teleportSync); | |||
} | |||
|
|||
MiNetServer.FastThreadPool.QueueUserWorkItem(SendChunksForKnownPosition); | |||
//MiNetServer.FastThreadPool.QueueUserWorkItem(SendChunksForKnownPosition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not doing this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chunks are often sent before the client can process them, because of this they are lost.
The most correct solution is to send them only after teleportation. After that, they will not go, since MINET thinks that the player has already received them.
src/MiNET/MiNET/Player.cs
Outdated
@@ -1176,7 +1303,8 @@ public virtual void Teleport(PlayerLocation newPosition) | |||
HeadYaw = 91, | |||
}); | |||
|
|||
ForcedSendChunk(newPosition); | |||
//ForcedSendChunk(newPosition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not doing this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this particular case can be returned, but I'm afraid to cache it, since the player may lose it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -1670,14 +1799,14 @@ public virtual void SpawnLevel(Level toLevel, PlayerLocation spawnPoint, bool us | |||
|
|||
SetNoAi(oldNoAi); | |||
|
|||
ForcedSendChunks(() => | |||
{ | |||
//ForcedSendChunks(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not doing this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chunks are often sent before the client can process them, because of this they are lost.
The most correct solution is to send them only after teleportation. After that, they will not go, since MINET thinks that the player has already received them.
src/MiNET/MiNET/Player.cs
Outdated
@@ -1659,7 +1787,8 @@ public virtual void SpawnLevel(Level toLevel, PlayerLocation spawnPoint, bool us | |||
|
|||
CleanCache(); | |||
|
|||
ForcedSendChunk(SpawnPosition); | |||
//ForcedSendChunk(SpawnPosition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not doing this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this particular case can be returned, but I'm afraid to cache it, since the player may lose it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -2236,6 +2294,23 @@ public void HandleMcpeSubChunkRequestPacket(McpeSubChunkRequestPacket message) | |||
SendPacket(response);*/ | |||
} | |||
|
|||
public virtual void HandleMcpeRequestAbility(McpeRequestAbility message) | |||
{ | |||
if (message.ability == 18) // flying?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have doubts this is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it important to fix this as part of this PR?
@@ -32,26 +32,46 @@ | |||
|
|||
namespace MiNET.Utils.IO | |||
{ | |||
//TODO - rework on NONE and ZLib instances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it you want to rework exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split into two implementations: without compression and with ZLib compression.
Since now before sending McpeNetworkSettings we should not use compression. And it's not the same as ZLib "NoCompression"
@@ -673,7 +676,7 @@ internal async Task UpdateAsync(RakSession session) | |||
|
|||
long elapsedTime = datagram.Timer.ElapsedMilliseconds; | |||
long datagramTimeout = rto * (datagram.TransmissionCount + session.ResendCount + 1); | |||
datagramTimeout = Math.Min(datagramTimeout, 3000); | |||
//datagramTimeout = Math.Min(datagramTimeout, 3000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, i still feel like it'd be good to atleast have A timeout to avoid filling memory with crap. Maybe make it configurable using the config.
@@ -84,7 +84,7 @@ internal void HandleOfflineRakMessage(ReadOnlyMemory<byte> receiveBytes, IPEndPo | |||
// Shortcut to reply fast, and no parsing | |||
if (messageType == DefaultMessageIdTypes.ID_OPEN_CONNECTION_REQUEST_1) | |||
{ | |||
if (!_greyListManager.AcceptConnection(senderEndpoint.Address)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to see this be put back as it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still waiting for an explanation why this shouldn't be, since the old implementation doesn't allow whitelist clients by Endpoint and not Ip but at least for me, this feature is critically important. It doesn't affect performance or anything else in any way.
@@ -68,7 +68,7 @@ public class HighPrecisionTimer : IDisposable | |||
public long Avarage = 0; | |||
|
|||
|
|||
public HighPrecisionTimer(int interval, Action<object> action, bool useSignaling = false, bool skipTicks = true) | |||
public HighPrecisionTimer(int interval, Action<object> action, bool useSignaling = false, bool skipTicks = true, bool highPrecision = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Highprecision should default to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you're right
but then the question is whether we need high precision in SkyLightCalculations.Calculate
https://github.com/NiclasOlofsson/MiNET/blob/df0d4ffb8c008654271e0c94187cada0abf6a30b/src/MiNET/MiNET/Worlds/SkyLightCalculations.cs#L182C4-L182C4
and may I set the highPrecision parameter from Config in the same way as it is done in Level for RakConnection SendTick timer? or does it need a different configuration name?
src/MiNET/MiNET/GreyListManager.cs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to see this reverted back.
Overall looking good at this point, just a few more things. Mainly the greylistmanager change from "ipaddress" to "ipendpoint" that one makes no sense to me. Could you answer my remaining questions? |
Is this still the 1.19 update? |
No description provided.