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

[WIP] Various fixes in Networking/FileIO packages #94

Merged
merged 5 commits into from Jul 31, 2018

Conversation

Projects
None yet
3 participants
@SamuelMoriarty
Copy link
Contributor

SamuelMoriarty commented Jul 21, 2018

No description provided.

// 4095 is 1111 1111 1111 in binary, one bit for each player slot
private static bitset allPlayers = bitset(4095)
// this sets allPlayers to a bit pattern of 1111..1111 where the total amount of 1's is equal to bj_MAX_PLAYER_SLOTS
private static bitset allPlayers = bitset(2 .pow(bj_MAX_PLAYER_SLOTS) - 1)

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jul 23, 2018

Contributor

will this be evaluated at compiletime?

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Jul 23, 2018

Author Contributor

I'll double check this. Will wrap in compiletime if not.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jul 23, 2018

Contributor

I think it will work either way, I'm just interested

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Jul 24, 2018

Author Contributor

Turns out it doesn't.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jul 24, 2018

Contributor

@Frotty might be interested to know that, thanks

@@ -17,7 +17,7 @@ import ErrorHandling
import StringUtils

/** Maximum amount of packets per single readable file. */
public constant PACKETS_PER_FILE = 16
public constant PACKETS_PER_FILE = bj_MAX_PLAYER_SLOTS

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jul 23, 2018

Contributor

Can you explain how this is related to player slots?

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Jul 23, 2018

Author Contributor

The current implementation relies on SetPlayerName to write/read packets to/from files. The previous limit was 16 'packets', equal to the number of players. Now that 1.29 supports 28 players, having it depend on the actual number of players as defined by the game better conveys the intent and is more reliable.

@SamuelMoriarty SamuelMoriarty changed the title Fix bit-pattern in SyncSimple for 24-player support [WIP] Various fixes in Networking/FileIO packages Jul 23, 2018

@SamuelMoriarty

This comment has been minimized.

Copy link
Contributor Author

SamuelMoriarty commented Jul 23, 2018

Don't merge this yet. I'm currently working on my map and discovering more subtle bugs in the libraries. Will update when my own map is working stable.

@@ -151,6 +151,9 @@ public function unit.getMoveSpeed() returns real
public function unit.getName() returns string
return GetUnitName(this)

public function unit.getProperName() returns string

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jul 23, 2018

Contributor

What's the behavior for non-heroes?

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Jul 24, 2018

Author Contributor

Simply returns null.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jul 24, 2018

Contributor

Is it documented in lep/jassdoc? Can you document it here? Is it right for this to be an extension function of unit?

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Jul 25, 2018

Author Contributor
  1. Jassdoc looks like it's hopelessly outdated. Someone needs to bring over all the new natives introduced into the latest patches there, but I am personally not up to the task. Just having one and not others would be weird.

  2. Other hero-related functions are exposed as extension funcs on top of unit, so I don't see any consistency issues here.

EDIT: Wait, this isn't a new native. Whoops. My second point still stands, however.

This comment has been minimized.

@Cokemonkey11

Cokemonkey11 Jul 25, 2018

Contributor
  1. I disagree about it being weird to have one but not others. If you don't have time to make a MR, can you at least raise an issue specifying that unit.getProperName returns null for non-hero?

  2. Ok

This comment has been minimized.

@Frotty

Frotty Jul 25, 2018

Member

Jassdoc looks like it's hopelessly outdated.

Why?
I don't think writing documentation about broken new natives, which get changed by pretty much any successive patch, has high priority.
Is there anything else outdated? I think jassdoc actually contains docs which cannot be found anywhere else. And unlike questionable pinned threads that only cover a small amount of natives in depth, this is an amazing, very structured and consistent overview.
And in its form it can be more easily used/referenced/updated/contributed to.

This comment has been minimized.

@SamuelMoriarty

SamuelMoriarty Jul 25, 2018

Author Contributor

I think the new natives are going to more or less stay in their current form from now on, apart from minor fixes to them, and it would be safe to include them and note any outstanding bugs, because some of them are quite useful.

Either way, I sent a PR to lep to update that particular function. I just thought it was one of the new natives - which, turns out, it isn't.

@SamuelMoriarty

This comment has been minimized.

Copy link
Contributor Author

SamuelMoriarty commented Jul 25, 2018

Some comments on my last commit.

  1. I added an event that gets executed after the LocalFiles check has been completed - this is necesarry because trying to use Persistable too early will result in an error.

  2. Fixed a desync in LocalPlayers on this line
    network.getData().writeBoolean(LocalFiles.isEnabled())

  3. Extended MAX_PACKET_LENGTH to 512 characters because it actually seems to work as intended. The max payload per file is now 14 kb thanks to 28 player slots!

  4. Turned bitset compilation in SyncSimple to compiletime.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Jul 31, 2018

Nice 🍭

@Frotty Frotty merged commit 3d716fe into wurstscript:master Jul 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment