-
Notifications
You must be signed in to change notification settings - Fork 2
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
Dynamically allocate arrays using MAX_SCREENWIDTH
/HEIGHT
#45
Conversation
It's not without its bugs: flat rendering is distorted and weapon sprites can be drawn cut off when changing resolutions.
If `cachedheight` is a dynamic array, that `memset()` call is problematic. We'll just keep it fixed-size for now.
Well, that was quick...
Hi, finally had the time to try the build resulting from this pr, here are the result. It seems that this PR has brought Nugget's performances in line with Woof 11.2
Hopefully when the optimizations from Woof will be merged in Nugget, the performance boost will compensate the costs of making 800p and 1600p available. I'm still curious about testing the current PR with
|
Hmm... I take it these tests were performed by warping to some maps and standing still, right? Any chance you could try some timedemos for each build? |
With the exception of pln.wad, all tests were timedemos (three runs each, average fps). win11 x64 |
I see, thanks. @ceski-1 thoughts? |
Looks like it's time to review |
for the record, the optimizations I've mentioned were introduced in Woof with these PRs: |
MAX_SCREENWIDTH
/HEIGHT
@@ -124,7 +124,7 @@ static void R_ClipWallSegment(int first, int last, boolean solid) | |||
|
|||
void R_ClearClipSegs (void) | |||
{ | |||
memset(solidcol, 0, MAX_SCREENWIDTH); | |||
memset(solidcol, 0, SCREENWIDTH << hires); // [Nugget] Dynamic arrays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to get away with viewwidth
whenever you use SCREENWIDTH << hires
in this code change.
BTW I re-run all timedemos because while testing the latest PR I noticed some inconsistencies.. All timedemos were ran 5 times (letting the cpu cool down in between runs). For PLN.WAD I simply warped into the level and stood still in one corner of the map, looking at the opposite corner. Noteworthy considerations:
In particular, the last point confirms what @MrAlaux was anticipating here #44 (comment) Sorry for the misleading previous results...
DEMOS: |
Thanks for this, @liPillON. It seems I was right about this PR's effects, then. If these changes are not improving performance in their current state, I don't think I'll be able to figure out how to make them do so. I'll try tweaking the allocation process as @fabiangreffrath suggested, but as it stands, that as far as I can go. Nugget 11.4 performing better than Woof 11.2 was a complete surprise, I wonder how that happened. The performance hit of allowing higher resolutions is certainly unwanted, but I guess we can live with it considering the unexpected performance gain over Woof, and also the fact that further down the road, Woof's latest optimizations will be merged. I might make a branch merging those optimizations early so you can test it. |
Sure! When the time comes, feel free to @ me in the discussion for issue 44 While we're here: what about benchmarking a |
I changed some things based on most of Fabian's suggestions, but to no avail it seems; the only change that seems to have increased performance was making I'm pretty sure that I'm the one doing something wrong here, and I'm certain I won't be able to figure out what exactly. As it stands, I can't do anything else with this branch, but I guess I'll leave the PR open for a while in case anyone wants to chime in. |
Alright, let's get some automated builds and test performance.