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

Document "Normal" Skybox #1955

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

Document "Normal" Skybox #1955

wants to merge 6 commits into from

Conversation

mzxrules
Copy link
Contributor

@mzxrules mzxrules commented Jun 9, 2024

No description provided.

SKYBOX_INDEX_HOLY_RESERVED_10,
SKYBOX_INDEX_HOLY_RESERVED_11,
// Error checking
SKYBOX_INDEX_INIT_MAGIC = 99, // Magic number used to detect a fault during initialization, value unused.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: IMO SKYBOX_INDEX_INIT_MAGIC -> SKYBOX_INDEX_INIT ("magic" makes me think of in-game magic, and really aren't all enum values magic numbers?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me what makes it magic is that the assigned value of 99 is set but never used by the program; it'd only show up when looking at a memory viewer.

@@ -66,6 +66,10 @@ typedef struct {

extern SkyboxFile gNormalSkyFiles[];

// Tests if the skybox palette for the given skybox should be written to color index 0-127.
// If false, it should be written to color index 128-255
#define IS_NORMAL_SKY_PALETTE_ALLOC_FRONT(skyboxIndex) ((skyboxIndex & 1) ^ ((skyboxIndex & 4) >> 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

the name IS_NORMAL_SKY_PALETTE_ALLOC_FRONT is mostly gibberish to me tbh but I don't really have any better ideas

Copy link
Contributor Author

@mzxrules mzxrules Jun 18, 2024

Choose a reason for hiding this comment

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

Perhaps it should be something more direct, like IS_NORMAL_SKYBOX_COLOR_INDEX_0/IS_NORMAL_SKYBOX_IMAGE_COLOR_INDEX_0?

@mzxrules
Copy link
Contributor Author

mzxrules commented Jul 7, 2024

Contributions made in this pr are licensed under CC0

Comment on lines +73 to +83
typedef enum {
// Clear Sky
SKYBOX_INDEX_CLEAR_SUNRISE,
SKYBOX_INDEX_CLEAR_DAY,
SKYBOX_INDEX_CLEAR_SUNSET,
SKYBOX_INDEX_CLEAR_NIGHT,
// Cloudy Sky
SKYBOX_INDEX_CLOUDY_SUNRISE,
SKYBOX_INDEX_CLOUDY_DAY,
SKYBOX_INDEX_CLOUDY_SUNSET,
SKYBOX_INDEX_CLOUDY_NIGHT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if we have a better descriptor than just "index" for this concept in general. Ill take a closer look at this and try to think about it more, but it makes reading all of the code pretty clunky to me.
Dont think "type" works cause this isnt necessarily the skybox type. But looking for something along those lines

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.

3 participants