Better handling for excess proximity lights#10241
Conversation
jonathoncobb
requested review from
david-c-kline and
keveleigh
as code owners
RogPodge
left a comment
RogPodge
left a comment
There was a problem hiding this comment.
I'm not really seeing where in the original code that that additional light data was discarded, as the capacity parameter of a list does not actually prevent more items for being added to the list
Removing the debug statement or making it only fire once does seem useful. Maybe we can instead case on
if (activeProximityLights.Count == proximityLightCount)
instead of
if (activeProximityLights.Count >= proximityLightCount)
so the warning is only filed once we pass the threshold, instead of continously? Otherwise I'm not sure I see the performance improvements.
|
|
||
| private static void AddProximityLight(ProximityLight light) | ||
| { | ||
| if (activeProximityLights.Count >= proximityLightCount) |
There was a problem hiding this comment.
Are there any concerns with having the activeProximityLights list grow unboundedly? I guess we're currently allowing it to grow unboundedly, while only throwing a warning, but just want to double check
There was a problem hiding this comment.
Sorry for letting this sit. I see the concern with this getting out of hand, though at a higher level, it's still bounded by the maximum number of pointers and thus light sources that will ever exist.
|
Hey @Cameron-Micka, you've been working on proximity light improvements. Is this something you've considered? |
I've not considered this but I like @jonathoncobb's idea! Makes sense to maintain the stack of lights so that the system can consider new lights in the order they were added when one of the first two slots frees up. |
Previously, if the shader's maximum proximity light limit was reached, a warning would be logged and the lights discarded. I've changed this to keep all lights around on the CPU side and only send the first two valid ones to the shader. This helps with race conditions where multiple lights are added and removed in a single frame and eliminates a warning that was spamming to the log and degrading performance.
jonathoncobb
force-pushed
the
proximity-light-queue
branch
from
092df05 to
711d83e
Compare
|
Hey @RogPodge. Sorry, I misspoke in the PR description when I said the lights were discarded. The main problem we were trying to solve was a spike that we were seeing when hands were raised and lowered and this warning was logged several times. If you're concerned about removing the logging entirely, perhaps it would be better if it were only in the editor since it's not particularly obvious when this warning is triggered on the device? |
|
@Cameron-Micka, can you ensure this gets ported to the v3 shaders? |
|
@RogPodge, can you also follow up with @jonathoncobb |
Of course! |
|
Reminder to self, we should be able to get this in for 2.8 :) |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
5 participants
Overview
Previously, if the shader's maximum proximity light limit was reached, a warning would be logged. I've changed this to keep all lights around on the CPU side and only send the first two valid ones to the shader. This helps with race conditions where multiple lights are added and removed in a single frame and eliminates a warning that was spamming to the log and degrading performance.