Position coin counter from right edge of screen

[nadiaholmquist]

Changes the currently fixed X coordinates of the coin counter HUD to use GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE so the counter will be properly right-aligned in widescreen like it is when playing in 4:3.

(Edit: this change was made before the relicensing - it's of course still fine to merge with the new license)

[mkst]

suggest this PR'd into sm64-port instead?

[nadiaholmquist]

I PR'd it here because positions of many other things and almost the entire HUD already use those macros, so the coin counter not being positioned using them seemed like an oversight. It would also help with widescreen hacks on the actual N64 which I've exprimented with doing myself.

If the PR is merged it should eventually make it into the PC port since they seem to want to keep up with upstream.

[chungy]

This repository is about making an exact representation of the original Nintendo 64 code. While you are providing a quality-of-life feature, it doesn't exist in the original game.

[nullstalgia]

Speaking as an outsider, given the point that it is a macro used in other UI elements, it would make sense to use a similar macro to ensure consistency. Plus, if it helps ports in the long run, that's just another plus.

[networkfusion]

I guess people are not getting it! @nadiaholmquist and @nullstalgia you are failing to understand the jist of this project, which is to create a 1 : 1 match to the original source for an n64 to be compiled and match byte for byte the original rom.

[nadiaholmquist]

No I understand that, with the changes I did the code still compiles into the exact same ROM as before, it only actually changes something if the screen resolution or the way those macros are implemented is altered. The goal here definitely is not to reproduce the original C code, as there are lots of features (like the base for widescreen support, options to avoid undefined behavior, making the code more portable than it would need to be for just running on the N64) that definitely would never have been in Nintendo's original codebase.

[Emill]

In my opinion this is more of personal preference. Some people would prefer in the center and some people more right aligned. Maybe this should rather be configurable.

[clavin]

This is a good change and does not change the compilation output. This adds clarity to the code¹ without compromising that core goal of compiling to the exact same binary output. There's some confusion about what this PR is doing, so here's a deep dive:

This PR only changes 3 magic numbers to be "calculated" using a macro named GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE:

- 168
- 184
- 198
+ GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(152)
+ GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(136)
+ GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(122)

The name is very telling of what it does, but here's its definition:

// in file /include/gfx_dimensions.h
#define GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(v) (SCREEN_WIDTH - (v))

I'd like to clarify that this is a macro and not a function. This macro is already part of the codebase, not added in this PR.

SCREEN_WIDTH is also a macro that already exists. Here's its definition as well:

// in file /include/config.h
#define SCREEN_WIDTH 320

Note: If you search in the codebase for SCREEN_WIDTH you'll notice that many constants are defined in terms of the screen width. For example, the skybox's width is defined as 4 * SCREEN_WIDTH. This isn't because those equations show up in the actual ROM, but rather because it's more clear where 4 * SCREEN_WIDTH comes from than 1280.

Since GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE is a macro, instead of generating a subroutine call, the compiler replaces the macro with its definition. That means the compiler will rewrite those new changed lines by "expanding" the macro to its definition:

GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(152)

// ... becomes ...

(SCREEN_WIDTH - (152))

// ... becomes ...

(320 - (152))

For the last stage, the compiler uses an optimization that replaces² the compiled (320 - (152)) into a constant 168, which is exactly what we started off with for that line.

(320 - (152))

// ... becomes  ...

168

So even though a macro is used now, the resulting binary is the same as we started with because the compiler is able to optimize it to be the same as the original.

As another note, this PR does not introduce any new features or goals for the project. That is, this project already has support for this notion of declaring values in terms of screen size. It is already mentioned in the current project source:

// in file /include/gfx_dimensions.h
/*

This file is for ports that want to enable widescreen.
...
*/

So this PR does not add any features. It refactors a magic number to explain where it comes from. It does not change the compiled binary nor change the game at all. It makes it easier when creating ports of the game from this codebase.

---

Notes

¹ Having just stumbled upon this project, seeing things like inline parameter comments (the /* position */ in some_fn(o->oAction, /* position */ 1, 2, 3);) has made navigating this codebase infinitely easier. Similarly, macros like GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(152) are much more contextual and clear than just a random number like 168.

² I actually went through the trouble of recompiling the project's "ido" compiler (which is used in this project to create binaries that are the same as the original ROM) in a docker container and then compiling the following example program:

// test.c

#define SCREEN_WIDTH 320
#define GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(v) (SCREEN_WIDTH - (v))

int main(void) {
    return GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(152);
}

In the compiled binary output, this was the result of that main function:

addiu v0, zero, 0xa8
jr ra

If you're not familiar with MIPS, that can be essentially read as "set the return value to 0xA8 and return." The hexadecimal value A8 is the decimal value 168, which is the result of GFX_DIMENSIONS_RECT_FROM_RIGHT_EDGE(152). The compiler indeed optimized the simple expression (320 - (152)) to its result 168. 🎉

Docker container test setup

My test was conducted in a directory structured with two files in it, a Dockerfile and test.c.

Dockerfile:

FROM ubuntu:18.04 AS base
WORKDIR /test
RUN apt-get update
RUN apt-get install -y build-essential git libcapstone-dev pkgconf
RUN git clone https://github.com/n64decomp/sm64.git

FROM base AS build_tools
RUN make -C /test/sm64/tools/ido5.3_recomp/
ENV PATH="/test/sm64/tools:${PATH}"

FROM build_tools
COPY ./test.c ./
# These flags I took from the project's makefile. I don't know what they all do.
RUN /test/sm64/tools/ido5.3_recomp/cc -c -G 0 -mips2 -g -non_shared -Wab,-r4300_mul -Xcpluscomm -Xfullwarn -signed -32 -o ./test ./test.c

The source of test.c is above.

To build the container and extract the compiled binary:

docker build -t sm64_test
docker run --rm --entrypoint cat sm64_test  /test/test > ./test.bin

Then the compiled binary is in the file test.bin. Using your favorite disassembler (with MIPS support), you can inspect that binary to see the main function.

If relevant, the current commit hash at time of testing is ecd3d152fb7c6f658d18543c0f4e8147b50d5dde.

[iProgramMC]

Curious.
Since the codebase already compiles byte-for-byte to the NTSC rom, why do you keep making changes?

changing names, documenting stuff, enhancements etc

[biggestsonicfan]

Okay, it's been over a year now since the 2nd approval. Would love to hear more on this.

[AlxBrx]

+1 for finally merging this. It seems only natural to me to use the Makro instead of magic numbers.

[Nightcaat]

Bump. This PR has been open for 900 days.

[biggestsonicfan]

Is there any reason this didn't make it into refresh 16 ae770e0?

[puigru]

This seems like a natural change and does not affect the compilation output. For all we know, this could have been in the original source code. Looking forward to it getting merged.

[mountainflaw]

I can't give a timeframe with the ways things are currently going, but I will make sure that this gets merged in for the next update. There are a couple last changes pending before we can move forward... Soon.

Apologies for the inactivity for the past three years, things have been very hectic for all of us, especially since this repo dropped right before the COVID outbreak.

[biggestsonicfan]

There was no reason to necro-approve this, @Algiuxs. It's been approved 3 times prior and will be merged in the next update.