Skip to content

Conversation

@Ikersfletch
Copy link
Contributor

Like vanilla, the CustomMemorialText assumes that the UI will be at 6x scale from the world.
When Level.Zoom =\= 1, this assumption fails, and the text is placed in a seemingly strange position.
This fix(?) makes the position more in-line with mapper expectations when the zoom factor =\= 1.

Note: this makes the custom memorial deviate from vanilla in behavior, which could theoretically break pre-existing maps' visuals. e.g.: If someone intentionally zoomed in for the odd text position, then this fix would break their map's visuals.
I don't think that's the case, but I think it's worth pointing out.

I also admit this is partially because people keep bugging me about these being broken with excam. Since I can't patch Everest with a code mod, this is the only way to fix it without duplicating them on my end.

Fixes unexpected text placement when level.Zoom =\= 1.
@maddie480-bot maddie480-bot added the review needed This PR needs 2 approvals to be merged (bot-managed) label Sep 1, 2025
Copy link
Member

@Wartori54 Wartori54 left a comment

Choose a reason for hiding this comment

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

Sounds like a very reasonable change to me.

If it does cause a significant impact on some maps we will correct be behaviour afterwards, since we do not have any good ways to check this before it gets rolled out. (But, of course, if any reviewer does know of any please do point those out.)

@JaThePlayer
Copy link
Member

I mean, if backwards compat is a concern, there's always the option to create a legacy option defaulting to true on old placements.

Copy link
Member

@microlith57 microlith57 left a comment

Choose a reason for hiding this comment

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

i agree with @Wartori54; this fixes a vanilla bug so is in scope for everest

@Wartori54
Copy link
Member

I mean, if backwards compat is a concern, there's always the option to create a legacy option defaulting to true on old placements.

My point is: we should avoid doing that if we can, and not do it potentially needlessly.

@maddie480-bot
Copy link
Member

The pull request was approved and entered the 3-day last-call window.
If no further reviews happen, it will end on Sep 7, 2025, 12:00 AM UTC, after which the pull request will be able to be merged.

@maddie480-bot maddie480-bot added last call window This PR was approved, and is in the 5-day last-call window before getting merged (bot-managed) and removed review needed This PR needs 2 approvals to be merged (bot-managed) labels Sep 1, 2025
@maddie480-bot
Copy link
Member

The pull request was approved and entered the 3-day last-call window. Since no PR should be merged within 3 days of the next rolling release, the last-call window is extended further.
If no further reviews happen, it will end on Sep 7, 2025, 12:00 AM UTC, after which the pull request will be able to be merged.

@SnipUndercover
Copy link
Member

Context: a bug has been spotted making the bot roll the "all-clear" confirmation back if the rolling release is less than 3 days, making the "last call window" be shorter in some cases.
The bot was made to re-evaluate all PRs.

@maddie480-bot
Copy link
Member

The last-call window for this pull request ended. It can now be merged if no blockers were brought up.

@maddie480-bot maddie480-bot added ready to merge This PR was approved and the last-call window is over (bot-managed) and removed last call window This PR was approved, and is in the 5-day last-call window before getting merged (bot-managed) labels Sep 6, 2025
@maddie480-bot
Copy link
Member

The pull request was approved and entered the 3-day last-call window. Since no PR should be merged within 3 days of the next rolling release, the last-call window is extended further.
If no further reviews happen, it will end on Sep 7, 2025, 12:00 AM UTC, after which the pull request will be able to be merged.

@maddie480-bot maddie480-bot added last call window This PR was approved, and is in the 5-day last-call window before getting merged (bot-managed) and removed ready to merge This PR was approved and the last-call window is over (bot-managed) labels Sep 6, 2025
@maddie480-bot
Copy link
Member

The last-call window for this pull request ended. It can now be merged if no blockers were brought up.

@maddie480-bot maddie480-bot added ready to merge This PR was approved and the last-call window is over (bot-managed) and removed last call window This PR was approved, and is in the 5-day last-call window before getting merged (bot-managed) labels Sep 7, 2025
@SnipUndercover SnipUndercover merged commit 4d359c5 into EverestAPI:dev Sep 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR was approved and the last-call window is over (bot-managed)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants