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

Add Speedometer #598

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add Speedometer #598

wants to merge 3 commits into from

Conversation

Jbudone
Copy link

@Jbudone Jbudone commented Dec 17, 2022

  • Introduced speedometer similar to JoeQuake, useful for speedrunners
  • Updated character draws to allow size specification
  • Added helpful calls -- find best matching color in palette; find hud size for offsetting

- Introduced speedometer similar to JoeQuake, useful for speedrunners
- Updated character draws to allow size specification
- Added helpful calls -- find best matching color in palette; find hud size for offsetting
Fixing clang format issues
@@ -566,6 +566,33 @@ void TexMgr_LoadPalette (void)
((byte *)&d_8to24table_conchars[0])[3] = 0;
}

/*
================
TexMgr_NearestColor
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't belong here. Has nothing to do with texture management.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed -- what would be a better spot for this? gl_draw.c maybe?

Copy link
Author

Choose a reason for hiding this comment

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

Or maybe common.c?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there plans to allow for changing colors? This seems to be only ever called with the pair of values (20, 20, 0), (40, 30, 15), so you could assume the default Quake palette is used and do bgColor = 51; fillColor = 19;. Otherwise gl_draw.c may be the best place.

@temx
Copy link
Collaborator

temx commented Dec 19, 2022

A few general comments:

  • This should be not enabled by default, especially if the cvar is not archived.
  • I haven't looked into this closely, but it seems to be doing its own positioning and text scaling. Would it be possible to use a canvas that already has the correct scaling? That way Draw_FillCharacterQuad_WithSize wouldn't be necessary, and maybe the positioning could be simplified too.
  • The vertical component of the speed is taken into account. I thought the standard was showing only the horizontal components (for example, JoeQuake shows a speed of 0 if you're jumping in-place).

@Jbudone
Copy link
Author

Jbudone commented Dec 19, 2022

The abstractions (_WithSize, _NearestColor) were intentional :) its only called here yes, but helpful for future newcomers like myself to not have to hardcode (eg. looking up palette and deciding color, or not being able to scale text)

@andrey-budko
Copy link
Contributor

I like the idea. scr_showspeed is the only reason I use my own fork. My implementation is simpler - just print the speed under crosshair.

andrey-budko@45245cf

image

@Jbudone
Copy link
Author

Jbudone commented Dec 29, 2022

Poke @Novum -- Is there anything else that needs to change?

  • I'd prefer to keep abstractions (_WithSize, _NearestColor) which feel like improvements to the engine
  • I can move TexMgr_NearestColor ; but where does that fit better? common.c maybe?
  • temx pointed out that I had it on by default (oops!) which I will fix in follow-up

static int fillColor = -1;
if (bgColor == -1)
{
bgColor = TexMgr_NearestColor (20, 20, 0);
Copy link
Owner

Choose a reason for hiding this comment

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

What's the point of this? Can't you just hard code the closest color from the palette? The palette is fixed as far as I know? Why does it even have to be a palette color?

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

Successfully merging this pull request may close these issues.

4 participants