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

Add FOV_3D_Z_RANGE option to limit the vertical range of 3D vision #31802

Merged
merged 4 commits into from
Sep 27, 2019

Conversation

kristleifur
Copy link
Contributor

@kristleifur kristleifur commented Jun 24, 2019

Summary

SUMMARY: Performance "Add adjustable 3D vision Z-level cap"

Purpose of change

So the 3D field of vision is pretty great. It's slow sometimes though, as is normal for experimental code. I find it useful to have an option to cap the depth of 3D vision. 5 levels is very playable on my machine and still adds a lot to gameplay. With full-depth 3D vision the game can become unplayable, e.g. in a decently sized city with some tall-ish buildings and a number of zombies.

Just having 1 level of 3D vision makes e.g. mall and rooftop gameplay a lot more immersive. I think that having this option will make it a little more feasible to have the 3D vision enabled always, which could tend to bring more developer attention to the 3D vision code itself as well as emergent stuff that plays against 3D vision.

Describe the solution

It's a very simple change that adds an integer option. That number is used to stop the vision calculations traveling up or down further than that many Z-levels. I set a default vision range of 4. I believe that this means that a total of 4 + 1 + 4 levels are seen, for a total of 9. (The game world is defined as 21 levels tall in OVERMAP_LAYERS.) Note that the default is still that the 3D vision is off, of course.

Describe alternatives you've considered

It might be interesting if we would add a timer that checks if 3D vision is taking too long and drops the number of levels. Either with a running check-and-cap, or with a time budget.

Ideally we want to get the 3D vision code running fast. I think this may be a useful step on the way there.

Additional context

Screenshot 2019-06-24 at 15 30 09

default 4.
* add logic to cast_zlight_segment that allows us to stop casting light
segments beyond FOV_3D_Z_RANGE levels. Decent speedup!
src/lightmap.cpp Outdated Show resolved Hide resolved
src/lightmap.h Outdated
@@ -3,6 +3,7 @@
#define LIGHTMAP_H

#include <cmath>
#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for? You did not change anything else in this headers, so why does it suddenly need this include?

Suggested change
#include <algorithm>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed necessary for std::min, but I’ll doublecheck :)

Copy link
Contributor

Choose a reason for hiding this comment

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

std::min is not used in this file. If you need it in some cpp file, include it there. There is no need to drag this include in for all files that happen to include "lightmap.h".

Co-Authored-By: anothersimulacrum <[email protected]>
@ZhilkinSerg ZhilkinSerg added Code: Performance Performance boosting code (CPU, memory, etc.) Z-levels Levels below and above ground. labels Jun 24, 2019
@kevingranade
Copy link
Member

Instead of a new option, can we perhaps change the existing option to be a value in the range 1-21?

@kristleifur
Copy link
Contributor Author

Certainly! I’ll rework!

@kevingranade
Copy link
Member

kevingranade commented Jun 24, 2019

Some further context, I've shelved the 3D vision optimization, which is WIP here master...kevingranade:fix-3d-shadowcasting because the performance problems that arose after the one second turn changes are more pressing, but I intend to return to that and related changes as soon as possible.
I fully believe that once I can get that change and some related optimizations done, we can remove the 3D vision option and leave it on all the time, which at some point will be mandatory for the game to work correctly as more 3D interactions are implemented.

@kristleifur
Copy link
Contributor Author

Cool! I'll attempt to be of assistance.

@ZhilkinSerg ZhilkinSerg self-assigned this Jun 27, 2019
@kristleifur
Copy link
Contributor Author

So I've been fretting a little over this after implementing the int-only option style.

No-Z-level vision vs. 3D-vision-that-looks-zero-levels-vertically are two very similar cases that have two different code paths. If we switch to an integer option, it gets a little clunky if we want to maintain the ability to test and compare 2D-vision and 3D-x-0-vision.

A way to do it is to make the value -1 mean "old 2D vision", and 0-21 be 3D vision with that many levels seen. But it feels a little awkward and unclear?

So given that we are hoping to develop the 3D vision soon, I think I would prefer the bool-and-int setup myself. It portrays the 3D vision as experimental, and the vision range as an experimental option thereon. Making "-1" mean "normal operation" feels like it might mislead users?

What do you think?

@kevingranade
Copy link
Member

Sure thing, it might be too confusing my way.

@esotericist
Copy link
Contributor

You should probably try to clamp how far a player can look up/down in x (look around) state based on this setting.

Right now you can set it to a value of 1, then hit x, <, <, and be looking at black space. (and you can even go further)

It also looks pretty weird when you get out of those bounds:
image

@kristleifur
Copy link
Contributor Author

Good catch! I’ll see if we could dynamically extend the range we’re peeking into or otherwise do even better than clamping.

@ZhilkinSerg ZhilkinSerg removed their assignment Jun 30, 2019
@kristleifur
Copy link
Contributor Author

Process-related update: Vacation time came before I managed to wrap this up :) I may not be able to work on this for a week or so. This is not forgotten though!

@stale
Copy link

stale bot commented Jul 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Jul 30, 2019
@kristleifur
Copy link
Contributor Author

kristleifur commented Jul 30, 2019

Thanks to whoever put that bot up! Thanks for the reminder. I'm getting this done this week.

edit: arrgh I overpromised. Had some family issues during the week. This is not forgotten :) <3

@stale stale bot removed the stale Closed for lack of activity, but still valid. label Jul 30, 2019
@ZhilkinSerg
Copy link
Contributor

Don't worry - we will be waiting.

@stale
Copy link

stale bot commented Sep 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Sep 20, 2019
@ZhilkinSerg ZhilkinSerg added (P5 - Long-term) Long-term WIP, may stay on the list for a while. and removed stale Closed for lack of activity, but still valid. labels Sep 20, 2019
@ZhilkinSerg
Copy link
Contributor

I've added option aware limits in look around mode and removed unnecessary include.

@ThomasLinkin
Copy link

Just want to ask how's the project. This one help alot in performance and it seems like until the #9664 test build i have been playing for the past few hours doesn't contain the option. Was wondering what happened

@kevingranade kevingranade merged commit f561e51 into CleverRaven:master Sep 27, 2019
@kristleifur
Copy link
Contributor Author

Thanks @ZhilkinSerg !!! <3 If I am to be technically accurate, then is a beautiful open-source moment for me personally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code: Performance Performance boosting code (CPU, memory, etc.) (P5 - Long-term) Long-term WIP, may stay on the list for a while. Z-levels Levels below and above ground.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants