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

split some graph.c related headers #1972

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mzxrules
Copy link
Contributor

No description provided.

include/graph.h Outdated Show resolved Hide resolved
include/graph.h Outdated Show resolved Hide resolved
Comment on lines +34 to +69
#define WORK_DISP __gfxCtx->work.p
#define POLY_OPA_DISP __gfxCtx->polyOpa.p
#define POLY_XLU_DISP __gfxCtx->polyXlu.p
#define OVERLAY_DISP __gfxCtx->overlay.p

#if OOT_DEBUG

// __gfxCtx shouldn't be used directly.
// Use the DISP macros defined above when writing to display buffers.
#define OPEN_DISPS(gfxCtx, file, line) \
{ \
GraphicsContext* __gfxCtx; \
Gfx* dispRefs[4]; \
__gfxCtx = gfxCtx; \
(void)__gfxCtx; \
Graph_OpenDisps(dispRefs, gfxCtx, file, line)
Copy link
Contributor

Choose a reason for hiding this comment

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

In MM we put all those macros in a different header, one that has the z_rcp.c function declarations because the files tend to use those macros and the rcp functions together.

I guess an argument can be made for the Open/CloseDisps and Alloc functions which are not present on MM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk it feels a little more sensible for these macros to exist in graph.h since gfxCtx is a graph.c stack variable.

I also did a once over and realized that most functions don't even need to be exposed outside of graph; the only oddball is ThreadEntry.

include/sys_matrix.h Outdated Show resolved Hide resolved
include/z64.h Outdated Show resolved Hide resolved
src/code/graph.c Outdated Show resolved Hide resolved
@mzxrules
Copy link
Contributor Author

mzxrules commented Jul 7, 2024

Contributions made in this pr are licensed under CC0

@mzxrules
Copy link
Contributor Author

sorry for the rebase, but there were extra files being shown as being modified that I didn't touch.

I'd like to recap where this PR is currently, explaining some of my decisions for what goes where and pointing out some new changes.

It was suggested that what is graph.h, gfxbuffers.h, gfx.h as defined in c1e57dc should be one header, but I think it's reasonable to keep them separate.

graph.h is meant to be specific to the graph thread and related tasks. The GraphicsContext is located here because it represents state important to the graph thread. WORK_DISP, POLY_OPA_DISP, POLY_XLU_DISP, OVERLAY_DISP and OPEN/CLOSE_DISP are here because they ultimately provide access to Graph_ThreadEntry's gfxCtx instance on stack.

gfx.h has had a lot of things split out of it, but I decided to keep it around to store the z_rcp.c types/function declarations because I feel it's just a little cleaner to do so. For one, main.h only needs the graph thread entrypoint not the drawing stuff, and two the z_rcp.c functions/dlists are not strictly necessary to do drawing. Currently, gfx.h includes graph.h just because I didn't feel like forward declaring GraphicsContext at the time. Not sure if I should keep it as is or not.

gfxbuffers.h is separate from graph.h because generally most things aren't allowed to access these buffers because the memory is being shared between the graph thread and the RCP. That said, I'm feeling less confident about this split.

As for other new changes.

I did a documentation pass on gfxalloc.c and standardized all uses of Gfx_Open/Gfx_Close. I'm not entirely sure where this belongs. While Gfx_Open/Gfx_Close are generic, all instances follow the same pattern of "opening" and "closing" POLY_OPA_DISP for writing to the point where there should be a macro that simplifies this in graph.h Perhaps this should be split off to a different PR altogether.

Introduced ucode_disasm.h, pretty self explanatory.

Copy link
Collaborator

@Dragorn421 Dragorn421 left a comment

Choose a reason for hiding this comment

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

graph.h and gfx.h are the same thing (both are "graphics") and shouldn't be split

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.

4 participants