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

Position coin counter from right edge of screen #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nadiaholmquist
Copy link

@nadiaholmquist nadiaholmquist commented Oct 28, 2020

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
Copy link

mkst commented Nov 4, 2020

suggest this PR'd into sm64-port instead?

@nadiaholmquist
Copy link
Author

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
Copy link

chungy commented Nov 8, 2020

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
Copy link

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
Copy link

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
Copy link
Author

nadiaholmquist commented Nov 8, 2020

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
Copy link

Emill commented Nov 8, 2020

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
Copy link

clavin commented Dec 23, 2020

This is a good change and does not change the compilation output. This adds clarity to the code1 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 replaces2 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

1 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.

2 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
Copy link

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

fjtrujy pushed a commit to fjtrujy/sm64 that referenced this pull request Aug 20, 2021
PS2: Fix compilation with new SDK
@biggestsonicfan
Copy link

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

@AlxBrx
Copy link

AlxBrx commented Apr 13, 2023

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

@Nightcaat
Copy link

Bump. This PR has been open for 900 days.

@biggestsonicfan
Copy link

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

@puigru
Copy link

puigru commented Sep 2, 2023

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
Copy link
Collaborator

mountainflaw commented Sep 2, 2023

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
Copy link

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

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.