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

Issue-46: Map Display #58

Conversation

Tysterman74
Copy link
Collaborator

Description

Button for Map Icon on main screen and display of map and rooms that has been generated in #30

Related issue(s)

Related to Issue #46

List of changes

A more detailed list of the changes.

  • Creation of MapUI Scene. This will be used for displaying the Map. Has a return button to go back to the main screen
    • Added logic to generate the rooms dynamically and scroll the map if need be.
  • Addition of Map Icon to TestingScene to open up the Map UI
  • Update Map Manager to hold the array of floors, and to create the 2D map when Map Manager is instantiated
    • Add Check if map has been instantiated
  • Update RoomBase to return the abbreviation of the type of room it is (used just for testing)
  • Added MapManager to autoloader in order to access the map_array within MapUI scene

Tests

Kind of stumped as to what to test here, some recommendations are welcome!

Additional notes

Vids of the map in different sizes:

map_display_1.mp4
map_display_2.mp4

@Turtyo Turtyo linked an issue Feb 2, 2024 that may be closed by this pull request
@Turtyo
Copy link
Collaborator

Turtyo commented Feb 2, 2024

This should close the map display no ? Since the light display is for another PR iirc

Map/RoomUI.gd Outdated Show resolved Hide resolved
@Turtyo
Copy link
Collaborator

Turtyo commented Feb 2, 2024

Things I have to say from pulling the PR alone (i'll review the code just after):

  • is it possible to make it so that the distance being scrolled is the exact distance between two floors on the display ? At the start, the lower border of the map fits perfectly between floor, but if the map is long it starts to go over rooms. It's just to try to make it look better
    Lower border is between the rooms
    image
    Lower border is not between the rooms
    image

  • Would it be possible to make the map larger ? The map can't handle more than 11 rooms of width
    image

  • Center the map inside the frame, the map is a bit skewed towards the right side, especially visible if you have a lot of rooms on a floor
    image

UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
@cheesefrycook
Copy link
Collaborator

Overall looks good! I think once we have the player's position represented on the map then we will want the scroll container to center on the player. Also, the scrolling feels a bit jarring at the moment. Turtyo mentioned making the tiers of rooms not be halfway cut off which might help. I'm also wondering if we can make the scrolling smooth instead instantly snapping.

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.

I don't know how it was added back, but I also notice that Card_DamageHealth.gd has been added back, doesn't seem to come from here, probably a merge in a previous PR but I didn't see where for now, didn't really search
Would be good to delete it, but it's a bit tricky
The best is probably that i do the push, but i will need to modify the deck for that; if you are ok with that Ty

Map/MapManager.gd Show resolved Hide resolved
Map/RoomBase.gd Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.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.

Thank you for the changes
I resolved comments for those that are good, I left open those that are not addressed yet

UI/MapUI.gd Outdated Show resolved Hide resolved
#Scenes/MapUI.tscn Show resolved Hide resolved
UI/MapUI.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.

I think the code is clearer now, I'll pull later to see how it looks now
This is making me think, should we have a minimum map size to make it look consistent ? If we have the final boss with only 1 room, it could look strange on the map. You calculate the width and choose the max between the min width and the calculated width

UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
@Tysterman74
Copy link
Collaborator Author

image

Here's how a [1, 1] Map looks like.
If we want to keep a minimum map size, I can add in logic to keep a minimum of array [1, 3, 5, 7, 5, 3, 1] and whatever that size is. That may be prove interesting to deal with since the map size is based on the 2D array we get back. The position may need to be updated.

…ardScrollUI and MapUI with the new "cancel_action", Add typing
@Turtyo
Copy link
Collaborator

Turtyo commented Feb 5, 2024

Yeah I think a minimum size would be good, as we can see small maps look a bit goofy

@Tysterman74
Copy link
Collaborator Author

Tysterman74 commented Feb 6, 2024

Here's a few examples of the minimum size map display, changes for this have been pushed up:

image

image

image

Let me know if there are any changes I can make.

UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.gd Outdated Show resolved Hide resolved
UI/MapUI.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.

Also adding some more comments based on the warnings I get when running the scene in godot

UI/MapUI.gd Outdated Show resolved Hide resolved
Map/MapManager.gd Outdated Show resolved Hide resolved
Turtyo
Turtyo previously approved these changes Feb 9, 2024
@Turtyo
Copy link
Collaborator

Turtyo commented Feb 9, 2024

I'm wondering, do we have a way to change the scroll speed ? with an export maybe ? i'm finding it a bit slow, but that might not be the case for everyone
I'm also thinking this might be a parameter for players later, so it might be good to have it exposed, so we can change it easily

@cheesefrycook
Copy link
Collaborator

Looks like scroll speed is an option on the smooth scroll container

@Turtyo
Copy link
Collaborator

Turtyo commented Feb 10, 2024

yes I saw it was an option, but i'm wondering where we are supposed to change it

@Tysterman74
Copy link
Collaborator Author

The SmoothScrollContainer already has it as an export on the inspector itself!

image

To your query about changing the speed, we can adjust the speed within code with SmoothScrollContainer.speed, this speed only adjusts scroll via the mouse scroll. The click and drag will be based on how fast the user flings the mouse when click and scrolling.

Once we get the save system squared away we can have a setting saved on that save file for adjusting player scroll speed. Just a potential thought.

@Turtyo
Copy link
Collaborator

Turtyo commented Feb 11, 2024

Seems good to me 👍

@Turtyo Turtyo merged commit 542fbc3 into Saplings-Projects:map-implementation Feb 11, 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.

Map display
3 participants