Skip to content
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

QoL: Increase playercount to 8 #6680

Closed
wants to merge 6 commits into from

Conversation

kphoenix137
Copy link
Collaborator

@kphoenix137 kphoenix137 commented Oct 1, 2023

image

@kphoenix137 kphoenix137 added the enhancement New feature or request label Oct 1, 2023
Source/lighting.h Outdated Show resolved Hide resolved
@AJenbo
Copy link
Member

AJenbo commented Oct 1, 2023

did you check what happens when players chat?

@ikonomov
Copy link
Contributor

ikonomov commented Oct 1, 2023

Dakk's meme comes to mind. Just because we could doesn't mean that we should. 8 seems like a random number. If we are to have this I think it should be done as an "advanced setting" or better yet, wait for the Lua. As it is I think this is a mod, not QoL.

@AJenbo
Copy link
Member

AJenbo commented Oct 1, 2023

I'm not sure lua would be able to do this since it's an engine limit. Having it as a setting would also require a lot of changes to begin with.

@kphoenix137
Copy link
Collaborator Author

did you check what happens when players chat?

Yes. nothing breaks. Just unable to mute the extra players. I think adding commands /mute and /unmute could be useful for this.

@kphoenix137
Copy link
Collaborator Author

8 seems like a random number.

Which number would not be a random number?

@kphoenix137
Copy link
Collaborator Author

Before undrafting, I'll also need to test out some other stuff, like town portal placement and golems. If anyone has ideas of anything that might break that I'm forgetting about, please share.

@AJenbo
Copy link
Member

AJenbo commented Oct 1, 2023

Basically the timedemo tests needs to pass for this to be merged, else I would have just merged #551 ;)

@kphoenix137
Copy link
Collaborator Author

Basically the timedemo tests needs to pass for this to be merged, else I would have just merged #551 ;)

So we just need a new timedemo? Is anyone interested in doing this? Last time I tried I couldn't get it working.

@StephenCWills
Copy link
Member

No, MAX_PLRS is used in Single Player and should therefore not be changed there or you'll break savegames. You need to make timedemo work without rerecording for this one.

@ikonomov
Copy link
Contributor

ikonomov commented Oct 9, 2023

8 seems like a random number.

Which number would not be a random number?

#551 Something like 16. It's as high as anybody would want to go.

@DakkJaniels
Copy link
Contributor

16 seems random to me, e^pi seems much less random.

@AJenbo
Copy link
Member

AJenbo commented Oct 9, 2023

Looks like the original WoW had a limit of about 4500 players pr world instance so we should probably have at least that

@kphoenix137
Copy link
Collaborator Author

I mean ikonomov makes a point; no reason to use a random number. Let's just make it uncapped

@yuripourre
Copy link
Collaborator

yuripourre commented Oct 11, 2023

This might be something stupid or maybe was fixed already but... IIRC, there is a limit of monsters, right? And because golems use the same array, if MAX_PLRS is too high it will allocate slots for golems and less monsters will be initialized, right?

Edit: Btw, if golems were moved to a separate array or list, could mods create summons?

@StephenCWills
Copy link
Member

This might be something stupid or maybe was fixed already but... IIRC, there is a limit of monsters, right? And because golems use the same array, if MAX_PLRS is too high it will allocate slots for golems and less monsters will be initialized, right?

You are absolutely correct. After this PR was submitted, I did some math to determine whether bumping the player count to 8 could have an effect on the number of monsters in a dungeon, and the answer is yes. However, even so, it didn't seem to me that occasionally having 4 fewer monsters would really be significant enough to bring it up. If we bumped the player count to 16, though, 12 fewer monsters is starting to sound significant.

Edit: Btw, if golems were moved to a separate array or list, could mods create summons?

Mods can create summons regardless. But putting Golems in a separate collection might make the process of introducing new ones a bit easier, since much of the code for it would probably be copy/paste.

@ikonomov
Copy link
Contributor

ikonomov commented Oct 12, 2023

The fact that changing this limit reduces the number of monsters per level is significant. Even if that number is just 4, in 16 levels that would be 64 less monsters. If this is to be merged I think this issue needs to be resolved so that there is no change of monsters per level. Also the mute functionality and the list of players in the control panel should be fully functional for the number of players, maybe it could be made scrollable.

Untitled-1

@StephenCWills
Copy link
Member

The fact that changing this limit reduces the number of monsters per level is significant. Even if that number is just 4, in 16 levels that would be 64 less monsters. If this is to be merged I think this issue needs to be resolved so that there is no change of monsters per level.

I could write a book on why this is a pretty flawed argument. But since you're advocating for no change at all on the basis that 4 fewer monsters is significant, I thought it might be more productive to simply ask how you'd feel about removing the upper limit of 186 monsters per dungeon level.

@ikonomov
Copy link
Contributor

OK, let me rephrase. It might not be significant, but it is not insignificant when considering a full level 1-16 run. I'm not advocating for no change at all, I'm just suggesting that perhaps reducing the number of monsters is an undesirable effect.

@StephenCWills
Copy link
Member

OK, let me rephrase. It might not be significant, but it is not insignificant when considering a full level 1-16 run. I'm not advocating for no change at all, I'm just suggesting that perhaps reducing the number of monsters is an undesirable effect.

So it sounds like you are not outright rejecting the idea of removing that upper limit. But let me be clear.

186 monsters is the upper limit on the number of monsters placed on each dlvl. Adding 4 more Golems reduces the upper limit to 182, which has the potential of reducing the total number of monsters by 4 in each dlvl. The actual number of monsters placed on each dlvl depends on the number of solid tiles in the dungeon's layout. I have no idea what a typical amount would be, and it probably varies a bit for each dungeon type, but the theoretical maximum (in an unrealistic scenario with a dungeon that has no solid tiles) before applying the upper limit is 213 in SP and 319 in MP.

@kphoenix137
Copy link
Collaborator Author

kphoenix137 commented Oct 12, 2023

Since a change of this magnitude isn't in response to a bug, and is in arbitrary change, I think its a good candidate for a Lua mod that can be included with the official installation

@AJenbo
Copy link
Member

AJenbo commented Oct 12, 2023

You can't really Lua you way out of engine limits like 4 player or 200 monsters.

@DakkJaniels
Copy link
Contributor

Yeah you'd basically have to make all the changes to support changing the number (maybe two separate vectors for monsters and golems, and whatever other supporting changes would be needed), and then just having lua let you modify the number of players that can join the game or something - I don't think this is a trivial change.

@kphoenix137
Copy link
Collaborator Author

kphoenix137 commented Oct 13, 2023

I think I have a solution for the mentioned issue, and also for fixing the problem with the single player file size being too large for reverse compatibility. Given that this change is specifically for multiplayer only, we can both allocate a monster array of size 204, and set the max players to 8 (given that this array can hold 200 values, I'm assuming then it's at least uint8_t, so we could move up to 256 unique values). We can use the same arrays for both but only save up the vanilla limit for single player save files and ignore the rest. As for loading single player saves we can load the data and fill the rest of the arrays with null values. But still for placing monsters, we would need to change the code to act as if we only have 200 indexes.

@AJenbo
Copy link
Member

AJenbo commented Oct 13, 2023

Existing saves will have monsters in index 5-8 so you also need to change how golems are allocated.
Doing this proper will likely require some more cleanup and rework on various parts.

@kphoenix137
Copy link
Collaborator Author

kphoenix137 commented Oct 13, 2023

Existing saves will have monsters in index 5-8 so you also need to change how golems are allocated. Doing this proper will likely require some more cleanup and rework on various parts.

The idea is to only allocate 4 slots for golems just like it currently works. Only multiplayer would allocate 8 slots for golems. I'd achieve this using 4 global defines for max players and monsters. Respectively 4 and 8, and 200 and 204. External arrays would use the multiplayer variant but only load/save using the single player defines. Game logic would be adjusted to check gbIsMultiplayer to decide which define to use.

@StephenCWills
Copy link
Member

If we want to go beyond 8 players, though, we should probably take some of these suggestions seriously. Like turning the arrays into vectors and moving Golems to their own collection.

@AJenbo
Copy link
Member

AJenbo commented Oct 13, 2023

And a scrollbar for the chat mute list.

@kphoenix137
Copy link
Collaborator Author

If we want to go beyond 8 players, though, we should probably take some of these suggestions seriously. Like turning the arrays into vectors and moving Golems to their own collection.

Wouldn't moving golems break reverse compatibility?

@AJenbo
Copy link
Member

AJenbo commented Oct 13, 2023

not if you map it

@AJenbo
Copy link
Member

AJenbo commented Nov 5, 2023

Proper support will require way more than is happening here, closing for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants