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

Weird camera distance when sitting on a bench #110

Merged
merged 2 commits into from Apr 3, 2021
Merged

Weird camera distance when sitting on a bench #110

merged 2 commits into from Apr 3, 2021

Conversation

szapp
Copy link
Collaborator

@szapp szapp commented Mar 31, 2021

Describe the bug
When the player is sitting on a bench, the camera zooms too much into the player.

Expected behavior
When the player is sitting on a bench, the camera no longer zooms too much into the player.

@AmProsius AmProsius added the validation: validated This issue is still present even with Systempack/Union. label Feb 11, 2021
@AmProsius
Copy link
Owner Author

INSTANCE CAMMODMOBBENCH (CCAMSYS_DEF)
{
bestrange = 1.20000000;
minrange = 0.500000000;
maxrange = 1.900000000;

changed to

INSTANCE CAMMODMOBBENCH (CCAMSYS_DEF)
{
     bestrange = 2.20000000;
     minrange = 1.500000000;
     maxrange = 2.900000000;

@AmProsius AmProsius added the provided fix This issue has a fix provided in the comments. label Feb 16, 2021
@szapp szapp added compatibility: easy This issue is easy to make compatible. type: session fix The fix for this issues is persistent across a session. labels Feb 16, 2021
@AmProsius AmProsius added the opinionated This issue or this issues' fix is opinionated and not predefined by the scripts. label Feb 18, 2021
@AmProsius AmProsius added this to To Do in v1.1.0 Feb 24, 2021
@AmProsius AmProsius added this to the v1.1.0 milestone Feb 24, 2021
@szapp szapp added this to Instance variable (int) in Fix templates Mar 17, 2021
@szapp szapp added compatibility: difficult This issue is difficult to make compatible. impl: unknown There is no clear plan on how to implement this issue yet. and removed compatibility: easy This issue is easy to make compatible. labels Mar 17, 2021
@szapp
Copy link
Collaborator

szapp commented Mar 17, 2021

This fix will be difficult, because the camera scripts are not parsed before entering the game (the first time in the session). Overwriting the instance function directly would be possible, but checking whether the fix is applied will not be easy in that case.

@szapp
Copy link
Collaborator

szapp commented Mar 17, 2021

@AmProsius Could you supply a screenshot of how that looks?

@szapp szapp self-assigned this Mar 17, 2021
@szapp szapp moved this from Instance variable (int) to Unsorted in Fix templates Mar 19, 2021
@AmProsius
Copy link
Owner Author

ScreenShot_2021_3_20_13_34_16

@szapp
Copy link
Collaborator

szapp commented Mar 20, 2021

I almost suspected that. I think this is just related to there being a wall behind the bench. Can you reproduce this odd camera angle for a free-standing bench? If not, it might be disadvantageous to change the camera distance.

@AmProsius
Copy link
Owner Author

It's the same camera distance for free standing benches:

ScreenShot_2021_3_23_15_28_35

If you want to check for yourself, the nearest waypoint is OCR_OUTSIDE_MCAMP_01.

@szapp
Copy link
Collaborator

szapp commented Mar 27, 2021

Although this fix does not have to be reverted, it will have to be applied after loading. Therefore I will mark it as savegame fix (i.e. revertible fit).

The idea is to overwrite the values directly in the created camera definition object (not the script instance). The respective address in memory will have to be found. This approach allows high compatibility (i.e. compatibility easy).

@szapp szapp added impl: change obj var This issue requires changing properties of objects. type: revert on save The fix for this issue impacts the game and should be reverted when saving. compatibility: easy This issue is easy to make compatible. and removed impl: unknown There is no clear plan on how to implement this issue yet. type: session fix The fix for this issues is persistent across a session. compatibility: difficult This issue is difficult to make compatible. labels Mar 27, 2021
@szapp
Copy link
Collaborator

szapp commented Mar 28, 2021

I compared the values to Gothic 2 and they are identical. The difference between Gothic 1 and Gothic 2 is, that the mouse input seems to be disabled in Gothic 1. Therefore, zooming out (by scrolling down) or rotating around the PC is not possible and the close camera becomes very apparent. As the camera distance itself was not changed in between Gothic 1 and Gothic 2 I would not consider it as a bug, but would try instead to enable mouse input while sitting on the bench.

Before I do that, @AmProsius could you confirm for me if your mouse input is also disabled while sitting on the bench (possibly with/without SystemPack/Union)?

@szapp szapp removed the compatibility: easy This issue is easy to make compatible. label Mar 28, 2021
@szapp szapp removed impl: change obj var This issue requires changing properties of objects. type: revert on save The fix for this issue impacts the game and should be reverted when saving. labels Mar 28, 2021
@AmProsius
Copy link
Owner Author

Before I do that, @AmProsius could you confirm for me if your mouse input is also disabled while sitting on the bench (possibly with/without SystemPack/Union)?

I can confirm that I also can't scroll when sitting on a bench (tested with and without Union).

@szapp
Copy link
Collaborator

szapp commented Mar 31, 2021

The fixed camera angle seems to be "indented": The mouse input is disabled for all MOB interactions except for ladders. In Gothic 2 this was changed. One could argue that it was neglected in Gothic 1 due to the last-minute mouse implementation, but then I wonder why ladders did receive special treatment.

I managed to "unlock" the mouse input for interaction with any MOB, i.e. camera instances that contain "CAMMODMOB" in their name, as Gothic 2 does it. Since this now happens for all MOBs, I don't know if it will introduce some unintended side-effects.

I would like to discuss whether to just replace the camera distance values for the bench (a direct bug fix, but given they are unchanged in Gothic 2, not the correct bug fix) or to unlock mouse input (going beyond a bug fix and with the risk of unintended side effects for other MOB camera definitions).

This commit is just a suggestion on how a fix could look like. If that
implementation is chosen, it should be rewritten in machine code for
performance. This here is just a preliminary implementation.

Refs #110
@szapp szapp marked this pull request as draft March 31, 2021 13:43
@szapp
Copy link
Collaborator

szapp commented Mar 31, 2021

I added a preliminary implementation to try it out and get a feel for it. If we choose to go with this option (unlocking the mouse input), the fix should probably be re-implemented with machine code. Until we have decided how to fix the issue, this here is very much work in progress and should not be merged!

@AmProsius
Copy link
Owner Author

I would like to discuss whether to just replace the camera distance values for the bench (a direct bug fix, but given they are unchanged in Gothic 2, not the correct bug fix) or to unlock mouse input (going beyond a bug fix and with the risk of unintended side effects for other MOB camera definitions).

Given that Gothic 2 changed the mouse behavior, I would like to go that path. The new values seem too opinionated for me. If we unintentionally create a new bug with enabling the mouse wheel, we can always go back to the fixed values.

szapp added a commit that referenced this pull request Mar 31, 2021
@szapp
Copy link
Collaborator

szapp commented Mar 31, 2021

I would like to discuss whether to just replace the camera distance values for the bench (a direct bug fix, but given they are unchanged in Gothic 2, not the correct bug fix) or to unlock mouse input (going beyond a bug fix and with the risk of unintended side effects for other MOB camera definitions).

Given that Gothic 2 changed the mouse behavior, I would like to go that path. The new values seem too opinionated for me. If we unintentionally create a new bug with enabling the mouse wheel, we can always go back to the fixed values.

Okay, sounds reasonable. I was working on the alternative (with explicit values). To not throw that away, I pushed that on a separate branch: bug110-alternative.

Now I will try to re-implement the solution here in machine code to make it feasible.

@szapp szapp moved this from Unsorted to Other in Fix templates Mar 31, 2021
@szapp szapp added compatibility: easy This issue is easy to make compatible. impl: modify engine func This issue requires modifying the machine code of engine functions. type: session fix The fix for this issues is persistent across a session. labels Mar 31, 2021
@szapp
Copy link
Collaborator

szapp commented Mar 31, 2021

I managed to implement it in a very light way. This fix should be tested thoroughly! That is, with many different MOBs to see if the newly allowed camera rotations do not interfere with anything. The implementation is on machine code level and now matches that of Gothic 2 quite closely.

To get it working, the mouse setting for MOBs is now hijacking the setting of ladders. I increased the X-movement from 0.5x speed to 1x speed (compared to the normal camera movement), because the ladder-camera is quite slow to move.

@szapp szapp marked this pull request as ready for review March 31, 2021 22:09
@szapp szapp removed their assignment Mar 31, 2021
@szapp szapp requested a review from AmProsius March 31, 2021 22:09
@szapp szapp moved this from To Do to In Progress in v1.1.0 Mar 31, 2021
Copy link
Owner

@AmProsius AmProsius left a comment

Choose a reason for hiding this comment

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

I couldn't find any problems with MOBs. It's really cool to be able to move the camera now!

@AmProsius AmProsius merged commit 99c0303 into master Apr 3, 2021
v1.1.0 automation moved this from In Progress to Done Apr 3, 2021
@AmProsius AmProsius deleted the bug110 branch April 3, 2021 19:47
szapp added a commit that referenced this pull request Apr 20, 2021
@szapp szapp moved this from Other to Engine mechanics in Fix templates Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility: easy This issue is easy to make compatible. impl: modify engine func This issue requires modifying the machine code of engine functions. opinionated This issue or this issues' fix is opinionated and not predefined by the scripts. provided fix This issue has a fix provided in the comments. type: session fix The fix for this issues is persistent across a session. validation: validated This issue is still present even with Systempack/Union.
Projects
Fix templates
Modify engine function
v1.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants