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

Backport 1.15's terminal rendering code #412

Merged
merged 8 commits into from Apr 21, 2020
Merged

Conversation

SquidDev
Copy link
Member

@SquidDev SquidDev commented Apr 20, 2020

This is a backport of 1.15's terminal rendering code with some further improvements. This deduplicates a fair bit of code, and is much more efficient.

I expect the work done in #409 will supersede this, but that's unlikely to make its way into the next release so it's worth getting this in for now.

  • Refactor a lot of common terminal code into FixedWithFontRenderer. This shouldn't change any behaviour, but makes a lot of our terminal renderers (printed pages, terminals, monitors) a lot cleaner.

  • Terminal rendering is done using a single mode/vertex format. Rather than drawing an untextured quad for the background colours, we use an entirely white piece of the terminal font. This allows us to batch draws together more elegantly.

  • Some minor optimisations:

    • Skip rendering "\0" and " " characters. These characters occur pretty often, especially on blank monitors and, as the font is empty here, it is safe to skip them.
    • Batch together adjacent background cells of the same colour. Again, most terminals will have large runs of the same colour, so this is a worthwhile optimisation.
    • Don't render monitors which are facing away from the player, and thus are invisible.

    These optimisations do mean that terminal performance is no longer consistent as "noisy" terminals will have worse performance. This is annoying, but still worthwhile.

  • Switch monitor rendering over to use VBOs.

    We also add a config option to switch between rendering backends. By default we'll choose the best one compatible with your GPU, but there is a config option to switch between VBOS (reasonable performance) and display lists (bad).

Numbers

Using the same setup as #356. I can't recall the exact monitor dimensions, but the image is 960x360. Here's a before and after:

Before

After

That big peak is due to the VBO uploading. It's still pretty bad, but is noticeably better than the current 1.12 and 1.15 code.

When this is more ready to merge, I'd also like to do some comparisons with smaller, but more frequently updating monitors. Graphics is not my forte, so I've no idea how it'll compare there.

If there's any rendering gurus out there, who have any additional comments, please do shout!

Monitors aren't done yet - I'm going to bed now, so will have a look
tomorrow morning.
 - Don't render the ' ' and '\0' characters. This should help a lot in the
   "common case", as monitors often have big chunks of blank space.
 - Combine adjacent identical backgrounds into a single quad. Again, as
   monitors often have areas of a solid colour, this should help
   dramatically.
 - Use VBOs for rendering monitors, instead of call lists. We'll
   probably need a fallback (and maybe config option), for those
   computers which don't support this.
@SquidDev SquidDev added enhancement An extension of a feature or a new feature. area-Minecraft This affects CC's Minecraft-specific content. labels Apr 20, 2020
@SquidDev SquidDev marked this pull request as ready for review April 20, 2020 12:55
@SquidDev
Copy link
Member Author

@Lignum Don't know if you have any comments on the above? I don't think there's anything unreasonable, but would appreciate a pair of eyes from someone who knows what they're doing if you've got time.

@Lemmmy
Copy link
Contributor

Lemmmy commented Apr 20, 2020

Minecraft doesn't support anything less than OpenGL 2.1. VBOs are core in that version, so there's no reason at all to even implement a direct or display list renderer

@SquidDev
Copy link
Member Author

SquidDev commented Apr 20, 2020

@Lemmmy I believe that restriction was only introduced in 1.15? That's what the changelog seems to suggest at least.

I agree it's unlikely that people have computers which don't support it, but also it's relatively little additional code.

That said, I could totally drop the direct renderer, given how useless it is.

Kinda added this for testing, can conclude it's terrible.
if( instance != null ) return instance;
return instance = new FixedWidthFontRenderer();
POSITION_COLOR_TEX.addElement( DefaultVertexFormats.POSITION_3F );
POSITION_COLOR_TEX.addElement( DefaultVertexFormats.COLOR_4UB );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the 4th colour component?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but BufferBuilder does! I want to stick with using this, as it's kinda unavoidable on 1.15 anyway, so might as well be consistent.

@Lignum
Copy link
Contributor

Lignum commented Apr 20, 2020

At this point, shouldn't we just rename FixedWidthFontRenderer to TerminalRenderer?

@SquidDev
Copy link
Member Author

Technically it's also used for printed pages. Though yes, the bulk of the code is for terminals.

@SquidDev
Copy link
Member Author

Reminder for myself tomorrow:

  • There's slight z-fighting at the edges of monitors. Not the end of the world, but worth seeing if it's fixable.
  • Skip rendering monitors which are facing away from the player. So just bog standard backface culling, but earlier in the process.

We may wish to improve this in the future - I need to dig into MC's
rendering code to check it's sane - but it's a good start.
Not sure how that got there.
@Lignum
Copy link
Contributor

Lignum commented Apr 21, 2020

That didn't go quite so well.

Not sure if GitHub will eventually decide to make that GIF work, but here's a direct link if not.

@SquidDev
Copy link
Member Author

Hah, that's what I get for writing code 6:30 in the morning. I'm fairly sure it's even more broken in 3rd person, as it uses the entity look vector rather than the actual render rotation.

I've got a fixed version locally, just need to test it and see what breaks.

This reverts commit 4d3cce4.

This would be really nice to have, but is sadly much trickier to
implement than I initially anticipated. Minecraft doesn't provide access
to the _actual_ camera, and given the odd things we need to take into
account (third person, "hurt" camera roll, etc...) I'm not going to
bother right now.

I'll look into this again at some point in the future - I'm 90% sure
it's possible, and if not on 1.12, then probably on 1.1{4,5}. Goodness
knows :).
@SquidDev
Copy link
Member Author

Heh, it's still broken. I've reverted it for now - I'll have another look when I've got some more time. It's an optimisation I imagine is worthwhile in something like SC's spawn, as you've got lots of monitors facing in other directions.

Either way, finally remembered to turn off vsync and FPS cap, and got some very happy screenshots:

2020-04-21_10 28 35

There's definitely some more performance which could be eked out of monitors, but at this point I'm fairly comfortable that most of the remaining FPS issues aren't CC's fault.

@SquidDev SquidDev merged commit 11bf601 into master Apr 21, 2020
@SquidDev SquidDev deleted the feature/monitor-vbo branch April 21, 2020 09:43
@SquidDev
Copy link
Member Author

Yeah, I'm going to regret that. Hey ho :).

@SquidDev SquidDev mentioned this pull request May 3, 2020
3 tasks
SquidDev added a commit that referenced this pull request May 4, 2020
Unknown blit colours, such as " " will be translated to black for the
background or white for the foreground. This restores the behaviour from
before #412.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Minecraft This affects CC's Minecraft-specific content. enhancement An extension of a feature or a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants