Add a command to convert existing local pins to server pins

Mydayyy commented 3 years ago

[x] Create a basic command to upload all existing local pins to the server, no exceptions
[x] Create an advanced command to upload all existing pins to the server:
- Do not upload pins to the server, when the server has a similar pin placed
- similar pin is decided by the radius alone, other factors (type, text) are ignored
[x] Create a command to delete all local pins

ghost commented 3 years ago

+1 for this suggestion. Not sure if it's possible, but if the command could detect nearby locally placed pins, to avoid duplicates, that would be even nicer 👍 . So if it detects a nearby local pin, delete the local one and sync up the ones placed on the server. So e.g. if a player joins the server, but already has their own locally placed markers in or around the same places, he won't get a bunch of duplicates.

Mydayyy commented 3 years ago

Greetings,

just converting local pins to server pins is pretty trivial and should easily be done.

About the duplicate thing: It would be possible too, but the question is what exactly identifies a duplicate pin. Which radius? Same type or not? Same text or not? and by that I fear that the user doesn't exactly know what he is getting when typing that command. It might very well just ignore local pins which are not duplicates but still close together. It might be useful to implement 3 commands, e.g:

/convertpins all /convertpins ignoreduplicates /deletellocalpins

The first and last command are pretty straight forward, but it will probably result in not everyone being happy with the detection of duplicate pins, so it will probably still require either cleanup of duplicate serverside pins or adding missing serverside pins after running that command.

I am planning to start developing the command feature tomorrow, maybe I'll get an idea. If you have any input Id be happy to hear :)

Cheers Mydayyy

ghost commented 3 years ago

Yeah, I'm not sure how well it would work either 😄

Which radius?

Great question. I have no good answer to this, because as you say, maybe a "close" radius would suit me & my friends, and wouldn't suit others.

Same type or not? Same text or not?

In my group of players, neither of us use the same icons, so that wouldn't work. Same goes for text.

The three commands would probably be a good idea.. Thinking the /deleteAllLocalPins might be the way to go for all players except one, and let the last one /convertpins all.

You could make the "detect duplicate" option optionable, so the default /convertpins would be ignoring the duplicates, and /convertpins removeLocalDupes could be the optional choice.

JTF195 commented 3 years ago

For the duplicate prevention, I would say just ignore any pins where the icon is overlapping when fully zoomed in.

Any pins that already exist server-side can be considered authoritative, and text can be fixed manually on the resulting single pin.

Another consideration: It might be wise to add some server-side configuration options to determine who is allowed to add/remove pins from the server. (Mainly important for the convert/import feature, but would be useful in general)

There is already an opt-in on the server and for each user, but I could see vandalism issues popping up on public servers unless an explicit allow list is also implemented.

JTF195 commented 3 years ago

Another couple of options to consider:

An option to silently ignore duplicate pins that are 100% overlapping, at all times. This would eliminate duplicates from other mods (such as Locator), which might not be able to use their own duplicate prevention mechanisms if the pins are stored server-side

An option to ignore pins from AutoMapPins altogether, as they are added and removed automatically in bulk and could place unnecessary strain on the server.

Mydayyy commented 3 years ago

Greetings,

I consolidated the ideas into a list of tasks to get a better overview:

Create a basic command to upload all existing local pins to the server, no exceptions
Create an advanced command to upload all existing pins to the server:
- Do not upload pins to the server, when the server has a similar pin placed
  - similar pin is decided by the radius alone, other factors (type, text) are ignored
Create a command to delete all local pins
Ability to enable permission and decide which players are allowed to add/remove/check pins #20
Sane behavior with pin placing mods (locator, AutoMapPins) #21

For the last two points I will create a new issue, the first three issues belong to this ticket and I will start working on those.

Cheers Mydayyy

Mydayyy commented 3 years ago

The three commands are implemented, currently living on branch 17-pin-commands. I made the duplicateRadius configurable and it defaults to 15.0.

I'll take a look at other pin mods tomorrow (locator, automappins) and try to work out a sane behavior for ssm.

I hope to be able to test friday/saturday and have a RC ready sometime during the day.

Cheers Mydayyy

Mydayyy commented 3 years ago

Pushed an RC2 today, with a small bugfix.

I also changed the command from /convertpins removelocaldupes to /convertpins ignorelocaldupes since I feel this conveys the actual action better. Remove would imply that the local duplicates are being removed by the command when in fact they stay but are just not uploaded to the server.

I didn't encounter any other issues while testing today so I hope I can push that out as a new version tomorrow.

JTF195 commented 3 years ago

I get the following errors when connecting to my dedicated server with WeylandMod.Core enabled on the client side Even with none of the other modules enabled on the client, and nothing but SSM on the server

[Info   : Unity Log] 03/16/2021 04:13:46: Exception in ZRpc::HandlePackage: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.IO.EndOfStreamException: Unable to read beyond the end of the stream.
  at System.IO.MemoryStream.InternalReadInt32 () [0x00034] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at (wrapper remoting-invoke-with-check) System.IO.MemoryStream.InternalReadInt32()
  at System.IO.BinaryReader.ReadInt32 () [0x00015] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at ZPackage.ReadByteArray () [0x00000] in <f48d6a22696245bf8d820aad6afa4fdb>:0
  at WeylandMod.Core.CorePlugin.ReadServerConfig (ZPackage pkg) [0x00000] in <5d99a99090024abab84c5c7864ac3b24>:0
  at MonoMod.Cil.RuntimeILReferenceBag+FastDelegateInvokers.Invoke[T1] (T1 arg1, MonoMod.Cil.RuntimeILReferenceBag+FastDelegateInvokers+Action`1[T1] del) [0x00000] in <9218caa36358471fb3c33e4432ee554c>:0
  at (wrapper dynamic-method) ZNet.DMD<ZNet::RPC_PeerInfo>(ZNet,ZRpc,ZPackage)
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <eae584ce26bc40229c1b1aa476bfa589>:0
   --- End of inner exception stack trace ---
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00048] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.Reflection.MethodBase.Invoke (System.Object obj, System.Object[] parameters) [0x00000] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.Delegate.DynamicInvokeImpl (System.Object[] args) [0x000e7] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.MulticastDelegate.DynamicInvokeImpl (System.Object[] args) [0x00008] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.Delegate.DynamicInvoke (System.Object[] args) [0x00000] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at ZRpc+RpcMethod`1[T].Invoke (ZRpc rpc, ZPackage pkg) [0x0001d] in <f48d6a22696245bf8d820aad6afa4fdb>:0
  at ZRpc.HandlePackage (ZPackage package) [0x00042] in <f48d6a22696245bf8d820aad6afa4fdb>:0
  at ZRpc.Update (System.Single dt) [0x0001e] in <f48d6a22696245bf8d820aad6afa4fdb>:0

[Info   : Unity Log] 03/16/2021 04:13:46: Exception in ZRpc::HandlePackage: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.NullReferenceException: Object reference not set to an instance of an object
  at ServerSideMap.InitialMapSync.SendChunkToServer (ZRpc client, System.Int32 chunk) [0x0007d] in <846274841e524258b727a23b8119320a>:0
  at ServerSideMap.InitialMapSync.OnReceiveMapDataInitial (ZRpc client, ZPackage mapData) [0x00089] in <846274841e524258b727a23b8119320a>:0
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <eae584ce26bc40229c1b1aa476bfa589>:0
   --- End of inner exception stack trace ---
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00048] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.Reflection.MethodBase.Invoke (System.Object obj, System.Object[] parameters) [0x00000] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.Delegate.DynamicInvokeImpl (System.Object[] args) [0x000e7] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.MulticastDelegate.DynamicInvokeImpl (System.Object[] args) [0x00008] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at System.Delegate.DynamicInvoke (System.Object[] args) [0x00000] in <eae584ce26bc40229c1b1aa476bfa589>:0
  at ZRpc+RpcMethod`1[T].Invoke (ZRpc rpc, ZPackage pkg) [0x0001d] in <f48d6a22696245bf8d820aad6afa4fdb>:0
  at ZRpc.HandlePackage (ZPackage package) [0x00042] in <f48d6a22696245bf8d820aad6afa4fdb>:0
  at ZRpc.Update (System.Single dt) [0x0001e] in <f48d6a22696245bf8d820aad6afa4fdb>:0

[Info   :ServerSideMap] Client received initial pin data by server. Pins: 0
[Info   :ServerSideMap] ClientAppendPins 0
[Info   : Unity Log] 03/16/2021 04:13:46: Session auth respons callback

[Info   :ServerSideMap] ClientAppendPins 0

This doesn't seem to happen when loading into a world locally and switching between characters. The map syncs, pins sync, etc

Mydayyy commented 3 years ago

Even with none of the other modules enabled on the client, and nothing but SSM on the server

Do you mean absolutely no plugins on the client and only SSM on the server?

Is that something that's happening now with RC builds or with the official release builds?

Cheers Mydayyy

JTF195 commented 3 years ago

It happens with rc2. I haven't tested release builds.

SSM on the dedicated server by itself

SSM, WeylandMod.Core and MMHOOK_assembly_valheim on the client (with ChangeVersion = false in the WeylandMod.Core config file)

with or without any of the other WeylandMod plugins,
with or without any other mods

Any other combination of mods without WeylandMod seems to work fine

Mydayyy commented 3 years ago

Greetings,

Yes, I can reproduce that error, but I also discovered the following:

When running no SSM at all, I still experience a crash when using no serverplugins and only WeylandCore on the client. Can you confirm that? Given that I would say this is not a SSM issue.

03/16/2021 21:38:29: Exception in ZRpc::HandlePackage: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.IO.EndOfStreamException: Unable to read beyond the end of the stream.
  at System.IO.MemoryStream.InternalReadInt32 () [0x00034] in <eae584ce26bc40229c1b1aa476bfa589>:0 
  at (wrapper remoting-invoke-with-check) System.IO.MemoryStream.InternalReadInt32()
  at System.IO.BinaryReader.ReadInt32 () [0x00015] in <eae584ce26bc40229c1b1aa476bfa589>:0 
  at ZPackage.ReadByteArray () [0x00000] in <dd568c2b4978432db77fc5f7f770f52b>:0 
  at WeylandMod.Core.CorePlugin.ReadServerConfig (ZPackage pkg) [0x00000] in <5d99a99090024abab84c5c7864ac3b24>:0 
  at MonoMod.Cil.RuntimeILReferenceBag+FastDelegateInvokers.Invoke[T1] (T1 arg1, MonoMod.Cil.RuntimeILReferenceBag+FastDelegateInvokers+Action`1[T1] del) [0x00000] in <e54299200d7d4726aec081e4ca77de7e>:0 
  at (wrapper dynamic-method) ZNet.DMD<ZNet::RPC_PeerInfo>(ZNet,ZRpc,ZPackage)
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <eae584ce26bc40229c1b1aa476bfa589>:0 
   --- End of inner exception stack trace ---
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00048] in <eae584ce26bc40229c1b1aa476bfa589>:0 
  at System.Reflection.MethodBase.Invoke (System.Object obj, System.Object[] parameters) [0x00000] in <eae584ce26bc40229c1b1aa476bfa589>:0 
  at System.Delegate.DynamicInvokeImpl (System.Object[] args) [0x000e7] in <eae584ce26bc40229c1b1aa476bfa589>:0 
  at System.MulticastDelegate.DynamicInvokeImpl (System.Object[] args) [0x00008] in <eae584ce26bc40229c1b1aa476bfa589>:0 
  at System.Delegate.DynamicInvoke (System.Object[] args) [0x00000] in <eae584ce26bc40229c1b1aa476bfa589>:0 
  at ZRpc+RpcMethod`1[T].Invoke (ZRpc rpc, ZPackage pkg) [0x0001d] in <dd568c2b4978432db77fc5f7f770f52b>:0 
  at ZRpc.HandlePackage (ZPackage package) [0x00042] in <dd568c2b4978432db77fc5f7f770f52b>:0 
  at ZRpc.Update (System.Single dt) [0x0001e] in <dd568c2b4978432db77fc5f7f770f52b>:0

(Logfile with naked server and only weylandCore clientside)

Cheers Mydayyy

JTF195 commented 3 years ago

I was able to reproduce the error under those conditions as well.

It does indeed appear to be an issue with WeylandMod itself.

JTF195 commented 3 years ago

When using /deletealllocalpins, I get the following error:

[Error  : Unity Log] InvalidOperationException: Collection was modified; enumeration operation may not execute.
Stack trace:
System.ThrowHelper.ThrowInvalidOperationException (System.ExceptionResource resource) (at <eae584ce26bc40229c1b1aa476bfa589>:0)
System.Collections.Generic.List`1+Enumerator[T].MoveNextRare () (at <eae584ce26bc40229c1b1aa476bfa589>:0)
System.Collections.Generic.List`1+Enumerator[T].MoveNext () (at <eae584ce26bc40229c1b1aa476bfa589>:0)
ServerSideMap.UtilityPin.DeleteLocalPins () (at <846274841e524258b727a23b8119320a>:0)
ServerSideMap.CommandsPinUpsync+ChatPatchInputText.Prefix (Chat __instance, UnityEngine.UI.InputField ___m_input) (at <846274841e524258b727a23b8119320a>:0)
(wrapper dynamic-method) Chat.DMD<Chat::InputText>(Chat)
Chat.Update () (at <f48d6a22696245bf8d820aad6afa4fdb>:0)

Tested under a bunch of different conditions. It will delete one pin and then throw the error.

Running the command subsequent times will continue to delete one pin at a time.

Mydayyy commented 3 years ago

Greetings,

are you running RC2? I think I addressed this already. This should only happen in RC1

Cheers Mydayyy

JTF195 commented 3 years ago

Yep. RC2

Mydayyy commented 3 years ago

Oh, I fixed a similar issue in RC2.

I just uploaded RC3 which addresses this: https://github.com/Mydayyy/Valheim-ServerSideMap/archive/rc3-1.3.0.zip

Mydayyy / Valheim-ServerSideMap

Add a command to convert existing local pins to server pins #17