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

Maniacs Feature - Pixel scrolling support for CommandPanScreen #3245

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

ToolMan2k
Copy link
Contributor

@ToolMan2k ToolMan2k commented Jul 14, 2024

This commit allows to pan the camera using X and Y coordinates.

  • Added support for .relative and .centered options for Absolute Pixel pan. Needed to bitmask parameter[4] in order to check for those.
  • Since Maniac Patch uses doubles for the current pan coordinates & speed, we copy the coordinates as a float in order to accurately round the speed value from double to integer.

Note:
There seems to be a bug on Maniac, when you have a looping map, you scroll up-left into negative coordinates, and you try to pan back to the center, more often than not it will keep panning diagonally indefinitely. Luckily EasyRPG doesn't have this issue, but for parity reasons we could try to reimplement that.

Sister request for EasyRPG/liblcf#482

Removing debug code, polishing code

+ Added support for .relative and .centered options for Absolute Pixel pan. Needed to bitmask parameter[4] in order to check for those.

= Renamed a few commands
= Added comments for some functions
- Removed debug code

Adjusting the speed formula

Now it works very well, even has differnet speeds for both H and V. However since Maniac internally uses doubles for screen panning, we might have to do some more work...

Revert "Adjusting the speed formula"

This reverts commit 52fa842.

Adjusting the speed formula Again

Now everything works as it should.

Time to use doubles for everything

Corrected interpolation, and proper scrolling speed

Correct formula for absolute positioning

Absolute Cam Panning now works

Changed some naming scheme

First things to do for Pixel Scrolling

Adding support for commands

However scrolling is a bit jaggy compared to ManiacPatch
@ToolMan2k ToolMan2k changed the title Maniac Patch: Adding pixel scrolling support Maniacs Feature - Pixel scrolling support for CommandPanScreen Jul 16, 2024
@fdelapena fdelapena added the Has PR Dependencies This PR depends on another PR label Aug 30, 2024
@Ghabry Ghabry added this to the 0.8.1 milestone Sep 5, 2024
@Ghabry Ghabry removed the Has PR Dependencies This PR depends on another PR label Oct 8, 2024
@fdelapena fdelapena requested a review from Ghabry October 11, 2024 02:39
@Ghabry
Copy link
Member

Ghabry commented Nov 21, 2024

sorry for the slow review process. Played around a bit with it and I think I have to try a newer version of Maniac.

The behaviour in my version when doing a relative scroll makes no sense:

@> Scroll Map: Set (px), (10, 20), 1000 * 0.001 px/f, (W), Relative
@> Scroll Map: Set (px), (10, 50), 1000 * 0.001 px/f, (W), Relative

In Maniacs this scroll once by (10, 20) and then scrolls once by (0, 30). wtf?!

In your implementation it scrolls by (10, 20), then by (10, 50). Which is imo correct 🤔


EDIT: My version was indeed too old. Works better with the latest Maniac Patch

@Ghabry
Copy link
Member

Ghabry commented Nov 22, 2024

Jenkins: test this please

Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Works for me. @ToolMan2k the only lacking implementation I found is when passing both relative and center. I could not figure out what Maniacs is calculating in this case. When you can solve this problem feel free to open a new PR.

@Ghabry Ghabry merged commit 24930fd into EasyRPG:master Nov 22, 2024
14 checks passed
@ToolMan2k
Copy link
Contributor Author

ToolMan2k commented Nov 23, 2024

Will do!

I'll check if I still have my test case (and modify it accordingly).

Edit: Will make a new PR soon™.

Edit2: Here's the thing: #3302

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants