search

SoftwareGuy / Ignorance

Ignorance utilizes the power of ENet to provide a reliable UDP networking transport for Mirror Networking.
Other
248 stars 31 forks source link

a few suggestions #19

Closed miwarnec closed 4 years ago

miwarnec commented 5 years ago

posting them all in one place for now.

image weird indentation

image weird indentation. also the 'costs?' is kinda weird. if you don't exactly know what it does then feel free to leave compression out of it for now. having an uncompressed enet transport is valuable already.

image whitespace

image remove this

image personally I wouldn't worry about buffering until ignorance is 1000% stable. telepathy doesn't buffer (yet) either.

image use dictionary.TryGetValue instead of containskey+lookup. faster and shorter.

image use trygetvalue

processservermessage: image copy exactly the received packet into a byte[] and then call onserverdatareceived.invoke with the exact packet that was received. don't copy the whole packetdatabuffer in there. not sure if you have to call incomingevent.packet.dispose either. C# will probably do that for you. unless there is a enet-sharp thing that require you to do that.

image https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.dictionary-2.remove?view=netframework-4.7.2 just remove, no need to check containskey first.

image this allocates a new arraysegment each time someone uses any mirror function with ignorance. probably better to go back to your old serversend without arraysegment. and if you still want arraysegment support then have another serversend function that does that.

image trygetvalue

image are you sure this works if I am on linux and build for osx? maybe this requires application.platform==osx check? (not 100% sure)

image copy directly into a byte[] with exactly the size that you need here too. less magic for now, stability is important, especially if you still have random errors in places.

image allocates a new arraysegment each time someone calls it from mirror. see above.

image usually better to do this with a git branch instead. but it's fine as long as you remove the old functions later.

image is this even needed if you only run ignorance from unity?

image imho a 'bool logDebug' is enough. people should always see warnings and errors.

image trygetvalue

image copy into a byte[] directly like explained above

image fix indentation

SoftwareGuy commented 5 years ago

This should be addressed in the latest commit(s), could you please check to see if there's any I missed from your list? Thanks.

martindevans commented 5 years ago

this allocates a new arraysegment each time someone uses any mirror function with ignorance. probably better to go back to your old serversend without arraysegment. and if you still want arraysegment support then have another serversend function that does that.

allocates a new arraysegment each time someone calls it from mirror. see above.

@vis2k This is totally fine - ArraySegment is a struct so constructing it is essentially free (it's as expensive to construct as 2 local integer variables). It doesn't cause any garbage to be created which is the only time allocations are a problem.

SoftwareGuy commented 4 years ago

I believe this is all resolved now. Going to close, please feel free to re-open if anything burning appears.