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

Better map event distribution #113

Merged
merged 25 commits into from
Sep 9, 2024
Merged

Better map event distribution #113

merged 25 commits into from
Sep 9, 2024

Conversation

Lann-hyl
Copy link
Collaborator

@Lann-hyl Lann-hyl commented Jul 15, 2024

Description

Changes Map Event generation based on set probabilities instead of just randomness.
There are also rules to prevent cases such as consecutive shop rooms, and floors where events are fixed (heal rooms before boss room).

Related issue(s)

Issue: #108

List of changes

Room events are generated with the following probabilities:

  • Mob: 45%
  • Random: 16%
  • Shop: 5%
  • Heal: 12%
  • Dialogue: 22%

The probabilities are set in GLOBAL_VAR.gd, and the implementation is in MapManager.gd

We then apply some generation rules that are inspired by this blog:

  • Rule 1: No Heal rooms before half of the map
  • Rule 2: No consecutive Heal and Shop rooms
  • Rule 3: There must be at least 2 room types among destinations of rooms that have 2 or more Paths going out.
  • Rule 4: No Heal rooms two floors before Boss (two before last floor)

Also some fixed rooms:

  • Last room is set to Mob (placeholder to be replaced by Boss at some point)
  • Rooms before boss are Heal rooms (before last floor)

Tests

Tests are written in test_map.gd, they test that the 4 rules described earlier are applied correctly to the test map.

Additional notes

Code in MapManager.gd that counts and prints the number of rooms for each event to verify if we get the number of rooms we want, with randomness. Set DEBUG_PRINT_EVENT_COUNT in DEBUG_VAR.gd to True.

@Lann-hyl Lann-hyl linked an issue Jul 15, 2024 that may be closed by this pull request
@Lann-hyl Lann-hyl self-assigned this Jul 15, 2024
Copy link
Collaborator

@Turtyo Turtyo left a comment

Choose a reason for hiding this comment

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

A few things to change, but it's mainly details
The comments explain well what you are doing, that's great

Would it be possible to add a test to check the time it takes to generate a map ? Ran multiple times, to know the distribution of times, maybe 100 times ?

Global/GLOBAL_VAR.gd Outdated Show resolved Hide resolved
Managers/MapManager.gd Outdated Show resolved Hide resolved
Managers/MapManager.gd Outdated Show resolved Hide resolved
Managers/MapManager.gd Outdated Show resolved Hide resolved
Managers/MapManager.gd Outdated Show resolved Hide resolved
Tests/test_map.gd Outdated Show resolved Hide resolved
Tests/test_map.gd Outdated Show resolved Hide resolved
Tests/test_map.gd Outdated Show resolved Hide resolved
Tests/test_map.gd Show resolved Hide resolved
Tests/test_map.gd Show resolved Hide resolved
Tomzkk
Tomzkk previously approved these changes Aug 4, 2024
Copy link
Collaborator

@Tomzkk Tomzkk left a comment

Choose a reason for hiding this comment

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

The generation and the rules seem good. I had to take time with some of them (3rd one specifically) but then realized it's probably because they are made for the specific map layout we decided on.

If the distributions look good (which is being discussed in other message from turtyo) then it should be good

Global/GLOBAL_VAR.gd Outdated Show resolved Hide resolved
Managers/MapManager.gd Show resolved Hide resolved
Global/GLOBAL_VAR.gd Outdated Show resolved Hide resolved
Turtyo
Turtyo previously approved these changes Aug 11, 2024
Tests/test_map.gd Outdated Show resolved Hide resolved
Tomzkk
Tomzkk previously approved these changes Aug 18, 2024
@Lann-hyl Lann-hyl dismissed stale reviews from Tomzkk and Turtyo via 586fd42 August 18, 2024 19:49
Copy link
Collaborator

@Turtyo Turtyo left a comment

Choose a reason for hiding this comment

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

Seems clear, will just test it before approving

Managers/MapManager.gd Outdated Show resolved Hide resolved
@Turtyo
Copy link
Collaborator

Turtyo commented Aug 19, 2024

What happens if we can't generate the map because of rules ? I don't see a retry of the generation

Managers/MapManager.gd Outdated Show resolved Hide resolved
@Turtyo
Copy link
Collaborator

Turtyo commented Aug 20, 2024

Once we have the map re-generation in case we it gets blocked, I think it'll be good for approval

Managers/MapManager.gd Outdated Show resolved Hide resolved
Copy link
Collaborator

@Turtyo Turtyo left a comment

Choose a reason for hiding this comment

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

Seems good, will just test that tomorrow, a thing to change regarding code structure I think, the double break with a flag is a bit strange

Turtyo
Turtyo previously approved these changes Sep 7, 2024
Copy link
Collaborator

@Turtyo Turtyo left a comment

Choose a reason for hiding this comment

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

Good job 👍

@Turtyo Turtyo requested a review from Tomzkk September 7, 2024 19:51
Tomzkk
Tomzkk previously approved these changes Sep 8, 2024
Copy link
Collaborator

@Tomzkk Tomzkk 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. Left one small comment but if you think it's okay to keep it there then we don't have to move it.

If the distributions appear good enough, then I think we are good to go with this.

Managers/MapManager.gd Show resolved Hide resolved
@Lann-hyl Lann-hyl dismissed stale reviews from Tomzkk and Turtyo via ffd8235 September 8, 2024 19:24
@Turtyo Turtyo merged commit ef72535 into Saplings-Projects:main Sep 9, 2024
1 check passed
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.

Better map event distribution
3 participants