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

[Community PR] Integrate merrp's lighting branch #5288

Open
wants to merge 168 commits into
base: upcoming
Choose a base branch
from

Conversation

AsparagusEduardo
Copy link
Collaborator

@AsparagusEduardo AsparagusEduardo commented Aug 30, 2024

Description

This PR integrates ntegrating #aarant's lighting-expanded-id branch.

Do NOT squash, as preserving history will help minimize merge conflicts for users that already merged the feature

Changes from the base branch:

  • Changed the BW Map Popup to update its time while active.
    • I did this first to have a way of debugging overworld exterior tinting over the day by locally forcing the Popup to not disappear.
  • Use Expansion's Fake RTC.
    • Changed tinting refresh rate to change alongside Fake RTC.
  • Use Expansion's constants (TIME_NIGHT instead of TIME_OF_DAY_NIGHT).
    • This allows for different tinting for Morning vs Evening.
  • Use Expansion's OW_TIMES_OF_DAYconfig (which allows users to set their own times by generation).
  • Option to enable lamp light "wobbling" (disabled by default), as it can be nausiating for some players. Fix Blinking Lights in Rustboro aarant/pokeemerald#41 will handle it instead.
  • Disable object event shadows

Images

mGBA_cdHELxiZt6

People who collaborated with me in this PR

  • @aarant for the original implementation and feature branch,
  • @Greenphx9 for their help merging to current expansion.

Discord contact info

AsparagusEduardo

@AsparagusEduardo AsparagusEduardo marked this pull request as draft August 30, 2024 00:33
@AsparagusEduardo AsparagusEduardo added new-feature Adds a feature category: overworld Pertains to out-of-battle mechanics labels Aug 30, 2024
@Pawkkie
Copy link
Collaborator

Pawkkie commented Sep 30, 2024

Hey Edu I have a suggestion for you that I've just remembered about. A while back a user in the Discord was asking about removing / controlling the shadows from merrp's branch, I know at least removing is in scope, here.

I've actually tried to isolate these before and the solution I gave them worked for their purposes, so it might work for ours. All of the Weather_SetBlendCoeffs(8, 12); can have the 12 changed to affect how dark the shadow is. In my version I added

#define BASE_SHADOW_INTENSITY                    16 // Ranges from 0 to 16, where 0 is black and 16 is transparent

so the lines ended up as

Weather_SetBlendCoeffs(8, BASE_SHADOW_INTENSITY);

and then you can control how intense they are or disable them altogether with just the one value. I'm not sure if this has a place in our version but I wanted to share in case it's useful :)

@AsparagusEduardo
Copy link
Collaborator Author

Hey Edu I have a suggestion for you that I've just remembered about.

Ah, thank you very much, this will indeed be very useful :D
I would probably invert this, as 0 = turned off makes more sense to me. What's your opinion?

@Pawkkie
Copy link
Collaborator

Pawkkie commented Sep 30, 2024

I would probably invert this, as 0 = turned off makes more sense to me. What's your opinion?

Yeah absolutely I also did that when I was isolating it, fully in favour :D

@AsparagusEduardo AsparagusEduardo marked this pull request as ready for review October 8, 2024 15:55
@AsparagusEduardo
Copy link
Collaborator Author

All items in the description have been addressed.
Ready for review!

Copy link
Collaborator

@Bassoonian Bassoonian left a comment

Choose a reason for hiding this comment

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

This is just a code review, and I'd like to do a full one again once all changes here are addressed. This doesn't test in-game behaviour. I also missed a few magic numbers I think that still need clarification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no reason we should be changing bin files.

data/maps/DewfordTown/map.json Show resolved Hide resolved
data/maps/EverGrandeCity/map.json Show resolved Hide resolved
data/maps/LavaridgeTown/map.json Show resolved Hide resolved
data/maps/LilycoveCity/map.json Show resolved Hide resolved
Comment on lines +1675 to +1681
TimeMixPalettes(1,
&gPlttBufferUnfaded[OBJ_PLTT_ID(paletteNum)],
&gPlttBufferFaded[OBJ_PLTT_ID(paletteNum)],
(struct BlendSettings *)&gTimeOfDayBlend[currentTimeBlend.time0],
(struct BlendSettings *)&gTimeOfDayBlend[currentTimeBlend.time1],
currentTimeBlend.weight
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style

Comment on lines +1712 to +1713
UpdateAltBgPalettes(PALETTES_BG);
UpdatePalettesWithTime(PALETTES_ALL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation

@@ -740,7 +740,7 @@ static u8 RotatingGate_CreateGate(u8 gateId, s16 deltaX, s16 deltaY)

template.tileTag = gate->shape + ROTATING_GATE_TILE_TAG;

spriteId = CreateSprite(&template, 0, 0, 0x93);
spriteId = CreateSprite(&template, 0, 0, 0x93); // 0x93 is above shadows (0x94)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this comment?

Copy link
Collaborator

@mrgriffin mrgriffin Oct 9, 2024

Choose a reason for hiding this comment

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

I'd be tempted to write this as 0x94 - 1 (similar to the 0x94 + 1 above)... Except I agree with your comment elsewhere about magic numbers. If we had #define OW_SHADOW_SUBPRIORITY 0x94 or whatever, then it's OW_SHADOW_SUBPRIORITY - 1 which maybe doesn't need the comment? Lower (sub)priority = above is consistent across the codebase.

EDIT: Actually I think 0x94 might be the priority of object events based on a comment earlier in the PR (I've added a comment there too).

Comment on lines +324 to +326
// TODO: Move these declarations into even_object_movement.h
#define OBJ_EVENT_PAL_TAG_MAY 0x1110
#define OBJ_EVENT_PAL_TAG_EMOTES 0x8003
Copy link
Collaborator

Choose a reason for hiding this comment

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

A TODO that needs to be addressed

Comment on lines +5337 to +5338
}
else if (show->threeCheers.sheen > 22)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, for pret or another PR

@Bassoonian Bassoonian added this to the 1.11 milestone Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: overworld Pertains to out-of-battle mechanics new-feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants