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

Add priority set / get and set_visible / is_visible to maps #563

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

screenshakes
Copy link
Contributor

@screenshakes screenshakes commented Feb 15, 2024

Adds .priority(), .set_priority() and .is_visible() and replace show and hide with .set_visible() in RegularMap, AffineMap and InfiniteScrolledMap.

  • Changelog updated / no changelog update needed

Copy link
Member

@gwilymk gwilymk left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the PR. I've left a few comments mainly around the doc comments :)

I don't see any harm in these, so if you need them for your game I see no issue in adding them. Maybe in the next version I'll change show() and hide() to be set_visible(bool) just for consistency

Comment on lines 50 to 51
fn show(&mut self);
fn hide(&mut self);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this now asks if this should be set_visible() but we can leave that for later :)

@@ -399,6 +399,23 @@ impl<'a> InfiniteScrolledMap<'a> {
self.map.hide();
}

/// Checks whether the map is not marked as hidden
Copy link
Member

Choose a reason for hiding this comment

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

could you link to show and hide in the docs for this? Just so if someone searches visible it'll take them to the methods which can change the state here :)

Comment on lines 274 to 288
// Gets the map priority
#[must_use]
pub fn priority(&self) -> Priority {
self.priority
}

/// Sets the map priority
pub fn set_priority(&mut self, priority: Priority) {
self.priority = priority;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think in the doc comments for these (and the same for InfiniteScrolledMap) it should mention how the priority won't change until you call commit()? But that priority() will return the most recently set value?

Just because hide() and show() happen immediately :)

Also could you link between the priority() and set_priority() methods in the doc comments so you can go between them easily?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's definitely worth mentioning!

@screenshakes
Copy link
Contributor Author

I agree that set_visible would be better not only for consistency but it will also remove the need for an if else in some usage.
I'll modify the PR and change that, I can do an other one later that will do the same to Object as well.

@screenshakes screenshakes marked this pull request as draft February 16, 2024 08:41
@screenshakes screenshakes changed the title Add priority set / get and is_visible to maps Add priority set / get and set_visible / is_visible to maps Feb 16, 2024
@screenshakes screenshakes force-pushed the map-visibility-priority branch from 7e21a26 to 2a7567c Compare February 16, 2024 10:17
@screenshakes screenshakes marked this pull request as ready for review February 16, 2024 10:21
@screenshakes screenshakes force-pushed the map-visibility-priority branch from 2a7567c to da84131 Compare February 16, 2024 10:21
Copy link
Member

@gwilymk gwilymk left a comment

Choose a reason for hiding this comment

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

Nice. Just one comment which I'll do now before merging :)

@@ -85,12 +85,12 @@ pub fn show_title_screen(background: &mut RegularMap, vram: &mut VRamManager, sf
background.set_scroll_pos((0i16, 0).into());
vram.set_background_palettes(backgrounds::PALETTES);

background.hide();
background.set_visible(true);
Copy link
Member

Choose a reason for hiding this comment

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

this should be set_visible(false)

@gwilymk gwilymk enabled auto-merge February 16, 2024 20:59
@gwilymk gwilymk added this pull request to the merge queue Feb 16, 2024
Merged via the queue into agbrs:master with commit e610a1c Feb 16, 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.

2 participants