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

Fix superflous shader setting updates #4800

Merged
merged 1 commit into from Nov 22, 2016

Conversation

ShadowNinja
Copy link
Member

@ShadowNinja ShadowNinja commented Nov 21, 2016

This can improve rendering performance by ~40%. (very hardware dependent)
Rebased version of #4530 with the following changes:

  • Free ShaderCallbacks instead of just leaking them.
  • Replaced the Sky** thing with a setSky() thing. It's still a bit ugly, but it works and it's maybe a little less ugly. (If you don't like it please offer a better solution)
  • Removed read of uninitialized memory.
  • Some minor style things that kwolekr noticed.

@ShadowNinja ShadowNinja added the WIP The PR is still being worked on by its author and not ready yet. label Nov 21, 2016
@Zeno-
Copy link
Contributor

Zeno- commented Nov 21, 2016

Since it's a WIP, I don't expect the memory-related problems have been addressed yet?

For the record, if a "factory" is the best approach (and I can't really think of a better one, but it seems confusing) then I'm ok with that.

Edit: SetSky() is similar to how I did it. I'm fine with that approach; it doesn't confuse things.

@Fixer-007
Copy link
Contributor

Fixer-007 commented Nov 21, 2016

It is 5th reincarnation of this PR, I hope you get it right this time once and for all. I guess I will use it daily.

@ShadowNinja
Copy link
Member Author

ShadowNinja commented Nov 22, 2016

@Zeno-: The compare with uninitialized values has been fixed, the memory leak has not been fixed (yet).

This improves rendering performance by ~40%
@ShadowNinja ShadowNinja removed the WIP The PR is still being worked on by its author and not ready yet. label Nov 22, 2016
@ShadowNinja
Copy link
Member Author

@Zeno-: Aaaaand fixed. (although I didn't have a few weeks to spare to wait for a valgrind run for confirmation...) This should be ready now.

@Zeno-
Copy link
Contributor

Zeno- commented Nov 22, 2016

👍

@Zeno- Zeno- merged commit 4bf4154 into minetest:master Nov 22, 2016
Zeno- added a commit that referenced this pull request Nov 22, 2016
This merge doesn't make any functional changes. It's a trivial style fix so that @gregorycu can be dual credited along with shadowninja for PR #4800
@Fixer-007
Copy link
Contributor

Congratulations to @gregorycu and everyone involved in this.

@gregorycu
Copy link
Contributor

Thank you @Fixer-007 . You were as involved as anyone else, so you should be pretty happy too!

@lhofhansl
Copy link
Contributor

It's awesome to see this in.
My son has my old laptop with an ATI card (can't check which one right now), and it feels noticeably faster there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants