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

RPG: Fix regression in smooth lighting animation on moving pit platforms. Other lighting fixes and improvements. #331

Merged
merged 9 commits into from
Nov 14, 2020

Conversation

mrimer
Copy link
Member

@mrimer mrimer commented Nov 14, 2020

Relevant topic

Multiple changes here. Check out the commit diffs individually for the recommended way to follow the changes.

If this looks good to you, I'll migrate these fixes to 5.1.1.

@EvidentlyCube
Copy link
Collaborator

Seems fine, I think I'll have to see it migrated to DROD codebase to better understand all the changes.

@EvidentlyCube
Copy link
Collaborator

I am reviewing it though! That was just first impression comment!

Copy link
Collaborator

@EvidentlyCube EvidentlyCube left a comment

Choose a reason for hiding this comment

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

Approving but I'd like a few more comments where I mentioned it. I think I get now how it's supposed to work, though I do worry a little bit that it's a very specific solution, and if we ever end up needing something similar for anything else it'll be a lot of work to make it work. But then again I think it's high time to get 5.1.1 out the door, let's avoid refactor creep.

FrontEndLib/BitmapManager.h Show resolved Hide resolved
FrontEndLib/BitmapManager.h Outdated Show resolved Hide resolved
@mrimer
Copy link
Member Author

mrimer commented Nov 14, 2020

I do worry a little bit that it's a very specific solution, and if we ever end up needing something similar for anything else it'll be a lot of work to make it work.

This is a good call out, and I don't disagree. This was a fair amount of work, and it is a specific solution. Still, we might find the new routine in CBitmapManager to be something that can be recycled for future complex rendering operations, even if we stop using it for this particular use case.

Fortunately, my solution to fixing the pit darkening is localized to one spot in CRoomWidget, so it would be easy to remove. If we need to generalize this type of operation (or go with something completely different) in the future, I have no qualms (in principle) about ripping that out and replacing it.

I agree we should get 5.1.1 out the door.

@mrimer
Copy link
Member Author

mrimer commented Nov 14, 2020

BTW, to check out the light rendering changes, they are easiest to observe if you slow down the key repeat rate and try moving around in a very dark room. Try also stepping down from a wall or top of a door for z-axis light tweening! This will also all work when migrated to 5.x.

@mrimer
Copy link
Member Author

mrimer commented Nov 14, 2020

Ready to review my latest commit.

@mrimer mrimer merged commit e0a7e26 into CaravelGames:master Nov 14, 2020
mrimer added a commit to mrimer/drod that referenced this pull request Nov 17, 2020
mrimer added a commit to mrimer/drod that referenced this pull request Dec 1, 2020
mrimer added a commit that referenced this pull request Dec 1, 2020
Fix regression introduced in PR #331: too much darkness applied to entities.
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