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

Turn on 3D FOV by default and combine its two options into one #71306

Merged
merged 9 commits into from
Jan 31, 2024

Conversation

Qrox
Copy link
Contributor

@Qrox Qrox commented Jan 28, 2024

Summary

Interface "Turn on 3D FOV by default and combine its two options into one"

Purpose of change

Closes #71185. Fixes #63276. Closes #69053. Closes #58601 since I cannot no longer reproduce it (but it may have already been fixed before this PR).

3D FOV has been working fine for a while and I finally managed to fix the unit test errors caused by turning it on, so let's do it.

The old colored blocks display of the level below can be restored by setting 'Vertical range of 3D field of vision' to 0, but I don't think it is worth supporting multiple-level vision with colored blocks display because the main reason for the colored blocks display is performance and multiple-level vision is going to affect performance anyway, and fewer options make it easier to maintain the vision code.

Here's a list of previous behaviors (from my memory, didn't bother to test so do correct me if I'm wrong) and current behaviors. Most of the previous behaviors are still supported.

FOV_3D FOV_3D_Z_RANGE Behavior & comments
Before multi-level rendering On 0 Colored blocks & no z-level vision (Still supported)
On non-zero Colored blocks & z-level vision (Not supported. Multi-levels are displayed instead. If performance is a concern, reduce the z-range value or change it to 0 to re-enable colored blocks)
Off 0 Colored blocks & no z-level vision (Still supported)
Off non-zero Colored blocks & no z-level vision (Changes into multi-level display)
After multi-level rendering On 0 Colored blocks & no z-level vision (Still supported)
On non-zero Multi-level & z-level vision (Still supported)
Off 0 Colored blocks & no z-level vision (Still supported)
Off non-zero Multi-level & no z-level vision (Not supported. Z-level vision is always enabled for consistent information.)
Now On 0 Colored blocks & no z-level vision
On non-zero Multi-level & z-level vision

Describe the solution

  1. Removed the FOV_3D option and code for fov_3d == false. Unified handling of fov_3d_z_range in the process.
  2. Fixed monster attack test by only allowing monster to stumble into the player when adjacent and properly implmenting the test for FOV_3D enabled.
  3. Fixed the player activity interruption test failure that has been haunting us, by forcing the weather to WEATHER_CLEAR.
  4. Fixed crash when moving or shifting view outside the z-level range.

Regarding 3; Turns out if the weather is changed due to previously run tests, the seen cache will be different. In this case the weather is mist, which seems to cause the player character to see through a wall column for a brief in-game second, which can also be reproduced in-game, and looks like #65205. The transparency cache also doesn't look right when the weather is clear in the test; z-level -1 seems to be considered to have the same transparency as air, although this problem does not happen in-game. I did not bother to fix these problems in this PR though; someone with more experience with the shadowcasting code should probably look into it.

Describe alternatives you've considered

Not doing this; Keep the FOV_3D option.

Testing

Ran the unit tests and they passed. Moved around in-game and the shadows looked reasonable. Tried to move character or cursor to z-level 10, they did not move and the game did not crash.

The filter that reproduces the player activity interruption test failure is [vehicle], activity_interruption_by_distractions.

Additional context

@github-actions github-actions bot added Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` Monsters Monsters both friendly and unfriendly. Melee Melee weapons, tactics, techniques, reach attack <Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc. astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 28, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 28, 2024
@PatrikLundell
Copy link
Contributor

I believe #69053 remains a relevant hindrance.

@Qrox
Copy link
Contributor Author

Qrox commented Jan 28, 2024

@PatrikLundell It seems the light map is only built on the player character's z level even with FOV 3D on, which caused NPCs to abort activities when the player character moves to a different z level. I worked it around by using the old behavior from non-3D-FOV mode, i.e. let NPCs continue activities when not on the same level as the player character. Building the light map for NPCs could be expensive, especially if there are multiple z levels with NPCs, so I didn't go that route.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 28, 2024
@andrei8l
Copy link
Contributor

The test npc_light_and_fine_detail_vision_mod can probably go. I made the wrong assumption there and it basically only checked that sunlight propagated down in open spaces.

Light map is only built on the player character's z level even with 3D FOV on, and building the light map on all NPCs' levels might be too expensive, so simply let NPCs finish their business when not on the player character's z-level for now, like what was done for non-3D-FOV mode.
@Qrox
Copy link
Contributor Author

Qrox commented Jan 29, 2024

@andrei8l I changed the test to use the test cases from non-3D-FOV mode, but I think we can keep the test to ensure that no one changes fine_detail_vision_mod without also reworking lightmap generation.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 29, 2024
…ed map

Test arguments that reproduce the failure in npc_can_target_player:

--rng-seed=1706562066 '~[slow] ~[.],starting_items'
@github-actions github-actions bot added the NPC / Factions NPCs, AI, Speech, Factions, Ownership label Jan 30, 2024
@Maleclypse Maleclypse merged commit a012bce into CleverRaven:master Jan 31, 2024
23 of 27 checks passed
@Qrox Qrox deleted the fov3d branch February 3, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions Melee Melee weapons, tactics, techniques, reach attack Monsters Monsters both friendly and unfriendly. NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
4 participants