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

Fix unicycle counting as car #288

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Fix unicycle counting as car #288

merged 2 commits into from
Feb 15, 2024

Conversation

sla-ppy
Copy link
Collaborator

@sla-ppy sla-ppy commented Feb 12, 2024

Fixes #246.

@sla-ppy
Copy link
Collaborator Author

sla-ppy commented Feb 12, 2024

This is a super dirty fix to the issue.
We essentially duct tape a problem which should definitely be revisited later on!

The reason for this dirty fix is because mVehichleData contains both unicycle and vehicle data, and I am unsure if we want it to stay like that.

I propose that in the new protocol we treat unicycle and vehicle data differently because there are questions to be answered like:

  • Do we want to allow players to spawn multiple unicycles (as controllable props) or do we want to have a single one representing and acting as the player itself?
  • Does it truly make sense to treat unicycles and vechicles differently?

Benefit of the proposal:

  • Counting the unicycle and vehicle counts separately could be toyed around with further.
  • Future wise the unicycle could have multiple models for players to have fun with.

@sla-ppy
Copy link
Collaborator Author

sla-ppy commented Feb 12, 2024

Please review my code: @lionkor.

@lionkor lionkor self-requested a review February 12, 2024 03:05
Copy link
Member

@lionkor lionkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, logic looks sound and we obv saw it work.

src/Client.cpp Outdated Show resolved Hide resolved
src/Client.cpp Show resolved Hide resolved
@sla-ppy sla-ppy requested a review from lionkor February 14, 2024 12:52
@Starystars67
Copy link
Member

Could I further this topic by suggesting that instead a configuration option be added to specify vehicle names that can be added so they bypass the vehicle limit.

This would be for things like the unicycle as default but can then be expanded to items like cones, bollards, concrete barriers or even pucks.

@lionkor
Copy link
Member

lionkor commented Feb 15, 2024

Could I further this topic by suggesting that instead a configuration option be added to specify vehicle names that can be added so they bypass the vehicle limit.

This would be for things like the unicycle as default but can then be expanded to items like cones, bollards, concrete barriers or even pucks.

I would ask you to make a new feature issue for that, because I'd also like to specify that properly before building it, and I wouldn't want to tack it onto this bug as I want this in the next release.

@lionkor
Copy link
Member

lionkor commented Feb 15, 2024

@sla-ppy btw in documentate unicycle dirty fix the correct form is document ;)

Copy link
Member

@lionkor lionkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@lionkor lionkor merged commit 9a0c9d3 into BeamMP:minor Feb 15, 2024
@sla-ppy
Copy link
Collaborator Author

sla-ppy commented Feb 15, 2024

@sla-ppy btw in documentate unicycle dirty fix the correct form is document ;)

catty

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

Successfully merging this pull request may close these issues.

Unicycle is counted as a vehicle if you do not have a vehicle spawned
3 participants