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

[Experimental] Drop fast faces #6903

Closed
wants to merge 7 commits into from
Closed

[Experimental] Drop fast faces #6903

wants to merge 7 commits into from

Conversation

numberZero
Copy link
Contributor

@Fixer-007 @paramat Can you test performance, please? This mostly affects CPU, and only solid and liquid nodes.
I plan to base several features on this part, but if it affects performance significantly I will have to rethink my decision.

@numberZero
Copy link
Contributor Author

Note: as a side effect, this fixes #6777 .
Note: Travis job errored, that’s its fault, not mine.

void MapblockMeshGenerator::drawSolidNode()
{
bool faces[6] = {0, 0, 0, 0, 0, 0};
static const v3s16 tile_dirs[6] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this you already have this in directiontables.h if i'm correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sadly no, the order is different, and it is important here.

@Wayward1
Copy link
Contributor

Wayward1 commented Jan 10, 2018

Upon initial testing, FPS seems to be a bit higher, but movement is significantly more choppy, dtime jitter tends to skyrocket, and there are significant visual artifacts.

OS: Linux Mint 18.3 x86
CPU: Quad core Intel Atom Z3736F
Graphics: Intel Atom Processor Z36xxx/Z37xxx Series Graphics & Display

Edit: I received the same results on my main device (minus the significant choppiness), that one being:

OS: Linux Mint 18.3 x64
CPU: Quad core AMD A8-5545M APU with Radeon HD Graphics
Graphics: Radeon HD 8510G

screenshot_20180110_143754
screenshot_20180110_143813
screenshot_20180110_143916

Note the dtime jitter here:
screenshot_20180110_144105

Edit: new high dtime jitter (on my main device):
screenshot_20180110_151254

@numberZero
Copy link
Contributor Author

numberZero commented Jan 10, 2018

Fixed glitches. Water glitches were not visible with waving water. Grass glitches were due to the face grass side is not tileable vertically; a nodebox would look the same.
screenshot_20180110_235121
But dtime jitter difference is sad.

@paramat
Copy link
Contributor

paramat commented Jan 10, 2018

Why drop 'fast faces' if that seems to be an optimisation?
What features do you have in mind to be based on this? It may not be worth doing this depending on the features you are considering.

@numberZero
Copy link
Contributor Author

@paramat Depends on how fast these faces actually are. If they are significantly faster than this simple solution, it doesn’t worth that.

@Wayward1
Copy link
Contributor

Wayward1 commented Jan 11, 2018

With further testing, FPS seems to be a bit lower, for me at least, hovering around 52 versus the latest Minetest at around 57. There are also visible lines between certain nodes, like default:snow and default:snow_with_grass.

Minetest master:
screenshot from 2018-01-11 08-55-52

This PR:
screenshot from 2018-01-11 08-54-20

@Fixer-007
Copy link
Contributor

Fixer-007 commented Jan 11, 2018

Did 4 fly straight tests over same newly gen mg7 terrain with PR and without (2 each), seems like minimal effect on rendering for me on Core i3-2120 3,3GHz, ATI X800GTO 256mb OpenGL 2.1, win7.
However, CPU usage seems higher, about 14-15% more cpu cycles with this PR.

default

@numberZero
Copy link
Contributor Author

@Fixer-007 Only 15%? I was afraid it would rather double CPU usage. Nevertheless, I added several optimizations, it should be significantly faster now.

Note: I just realized it computes lighting wrongly. Fixing that is possible but is non-trivial.
Note: I can’t test performance/stutter/etc. myself as my new CPU is too fast, it can handle way more load.
Note: This PR is experimental and may not be a good decision.

@numberZero numberZero changed the title [Part] Drop fast faces [Experimental] Drop fast faces Jan 11, 2018
@rubenwardy rubenwardy added WIP Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Jan 11, 2018
@rubenwardy
Copy link
Contributor

What's the benefit and what PRs are relying on this?

@numberZero
Copy link
Contributor Author

No PRs currently rely on this.
The benefit should be, less weird and fragile code amount. The benefit should be, reusing of same code where appropriate.
By the way, I found some interesting details that might be handy to fix by themselves, like nodeboxes not supporting non-tiling textures.

@Fixer-007
Copy link
Contributor

Fixer-007 commented Jan 11, 2018

Only 15%?

That's according to Process Explorer figures. CPU cycles are good as benchmark, right? I don't use profilers or other fancy stuff.

@lhofhansl
Copy link
Contributor

So. Much. Simpler. :) I definitely like that part of it.
Going to try it - although I also have a pretty fast machine.

@lhofhansl
Copy link
Contributor

I see a measurable drop in FPS. Without the patch FPS is capped at 60 (have not tried with a higher limit), with the patch it hovers around 49.

If that's typical we should keep fast faces.

@numberZero
Copy link
Contributor Author

@lhofhansl The reason for lower FPS may be lack of tiling, as the mesh my PR produce should be identical otherwise, and meshgen time shouldn’t affect FPS much. On my GPU, that does not affect FPS at all, vertex shaders take way less time than fragment ones. On your GPU the situation seems to be different.
Note: I suppose you tested in vanilla MTG (or minimal), full of flat solid areas. On messy landscape the FPS difference should be lower.
Nevertheless, this appeared to be a bad idea; fast faces should be improved and not dropped. Closing.
P.S. Thanks for testing.

@numberZero numberZero closed this Jan 14, 2018
@lhofhansl
Copy link
Contributor

@numberZero Yeah, vanilla MTG. This was a "scene" with water, some islands, and some mountains visible.

@lhofhansl
Copy link
Contributor

@numberZero I'm looking at this one again, as fast faces prevent many vertex shader effects, since you get lines between long faces.
I wonder if the rational for fastfaces still holds today. I think removing them would lead to more draw calls, right? That would be bad.

@numberZero
Copy link
Contributor Author

I think removing them would lead to more draw calls, right? That would be bad.

No-no, draw call number is not affected. Vertex number would be higher, sometimes (on plains) significantly.

@lhofhansl
Copy link
Contributor

I see, not reducing the number of meshes, just the (average) count of vertices per mesh.

I'm going to try this again. The patch obviously does not apply anymore. I assume you did not keep a current version... (Had to ask, just in case.)

Tiling is already turned off for water (when waves are enabled).

@numberZero
Copy link
Contributor Author

numberZero commented Nov 30, 2020

@lhofhansl to test rendering performance, just forcing next_is_different (if it still has that name...) to true in the fastface generator should be enough.

@lhofhansl
Copy link
Contributor

@numberZero I did that already :) I see the extra vertices rendered (F6 shows 8.8m instead of 6.9m), but FPS stays the same (31 FPS). This is a new laptop, so not sure whether that is typical. I'll play with it some more.

@numberZero
Copy link
Contributor Author

Minetest is probably limited by drawcalls and not rendering itself on any more-or-less modern discrete GPU (except of maybe some nVidia® cards under nouveau which just can’t get them off the power-saving mode), and the former is mostly unaffected by fast-faces. But there are also integrated GPUs...

@lhofhansl
Copy link
Contributor

And here I am looking at this again. This time for large view_ranges, where the fastface (getTileInfo) is the CPU hog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants