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

Random stuff, again #349

Merged
merged 33 commits into from
Nov 2, 2023
Merged

Random stuff, again #349

merged 33 commits into from
Nov 2, 2023

Conversation

Mr-Auto
Copy link
Contributor

@Mr-Auto Mr-Auto commented Oct 20, 2023

No description provided.

if (theme == theme_arena) // no reason to?
continue;

hook_vtable<void(ThemeInfo*), 0x15>( // spawn_transition
Copy link
Collaborator

Choose a reason for hiding this comment

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

So why does this work but not the thing in hook_themes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And even more curious why the other hooks there do work but not this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to check but can't setup any debugging or export .pdb file (to use symbols in x64dbg) for release.
Was thinking maybe it's a thread issue, but changing ptr to ptr_main in get_entities doesn't solve this
So until you get to ask @Malacath-92 about this it remains mystery

Comment on lines +77 to +81
// green value is randomized in game
// uses PRNG_CLASS::LEVEL_DECO
// green = (PRNG::random_float / 1000 * 0.3 + 0.2) / 1.62

using construct_illumination_ptr_fun_t = Illumination*(std::vector<Illumination*>*, float*, float*, uint8_t, float, uint32_t, uint32_t, uint8_t);
static auto construct_illumination_ptr_call = (construct_illumination_ptr_fun_t*)get_address("generate_illumination");
float light_size = 0.6f; // should be 0.8f for dark level
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe i should implement this game behavior?

Comment on lines -332 to -339
"25: In back layer",
"26: ",
"27: ",
"28: ",
"29: ",
"30: ",
"31: ",
"32: ",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The UI was using that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By "using" you mean there is essential code that checks this "flag" or just a display thing?

it's kind of pointless and i don't want to encourage people to check some flag when it's just a bool value that can be checked way easier than the test_flag(state.special_visibility_flags, 25)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the only reason these char arrays exist is to print labels in ui.cpp, and you just removed part of the array without fixing the ui code... Now the ui just prints values from basecamp_dialogue_win_flags instead.

I know it's wrong, but it's still a funny thing to commit without fixing the ui code too. Changing the ui to only show checkboxes for the 6 flags in the 3 8bit variables would be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That template sure makes it simpler, but you also made some of the flag lists freakishly long, cause the for loops used to be shorter than the array. Yeah I know lazy arrays, but they are all from a time when we didn't know anything about anything.

Also there are a ton of unused flags between all the lists that ideally shouldn't be shown in the ui, cause things like presence flags and quest flags are also clearly multiple 8bit flags in a trench coat with a lot of unused flags in between. Unless those empty flags are used for something, but after a few years I think we would have documented them. I don't really care if some of these 32bit flags are actually multiple 8bit flags, as long as it's the whole 32 bits that is flags. It just makes the api more confusing to start slicing them all up now.

Copy link
Contributor Author

@Mr-Auto Mr-Auto Oct 30, 2023

Choose a reason for hiding this comment

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

The issue is, inconsistency in the flags.hpp. Some flags are empty (just number), other named "Unused" and some named "Unknown"

I can make the render_flags skip those if we really want to. Or add extra limit parameter to not show all of the fields

Also from quick look i think only journal_flags and presence_flags got longer

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably

  • make render_flags skip the whole checkbox if the name is empty
  • remove numbers from flag names but
  • add bool argument to print the numbers anyway, when knowing the flag number seems relevant
  • change the few actually used but empty names to ???, unknown or best guess
  • empty out the definitely unused names
  • (add still missing flag enums to api, so flags.hpp is not essential for documentation)

Copy link
Collaborator

Choose a reason for hiding this comment

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

template <std::size_t SIZE, typename T>
void render_flags(const std::array<const char*, SIZE> names_array, T* flag_field, bool show_number = true)
{
    for (int idx{0}; idx < SIZE && idx < sizeof(T) * 8; ++idx)
    {
        // just simplified version of CheckboxFlagsT

        T value = (T)std::pow(2, idx);
        bool on = (*flag_field & value) == value;
        if (names_array[idx][0] != '\0' && ImGui::Checkbox(show_number ? fmt::format("{}: {}", idx + 1, names_array[idx]).c_str() : names_array[idx], &on))
        {
            *flag_field ^= value;
        }
    }
}

and

std::array presence_flags{
    "Udjat eye",
    "Black market",
    "Vlad's castle/drill",
    "",
    "",
    "",
    "",
    "",
    "Moon challenge",
    "Star challenge",
    "Sun challenge",
};

turning into

image

Flag numbers should probably usually be shown, cause it's pretty common to at least talk about them using 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.

Few notes:
basecamp_dialogue_win_flags - not used in UI
movable_entity->buttons - not editable in UI, would be good feature, but it requires virtual function hook right?
char_states - don't have numbers, when i feel they should?
pause_types - have flag values, not flags numbers, should probably change to numbers for consistency right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also some stuff is named _flags then it's not really a flag, just an array or something

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • basecamp_dialogue_win_flags: I guess someone just documented it there, but the whole dialogue thing seemed pretty useless or boring for modding and we never made any api for it
  • buttons: yeah and that would be annoying to do in ui. editing buttons for most monsters isn't even very useful, cause changing states is usually how they move and attack. it's really just a leftover from investigating how monsters even work and can we control them
  • char_states is used next to an editable input with the number
  • pause: maybe, but then again the PAUSE enum is also just masks instead of flag numbers
  • _flags: lazy copy pasting

@Mr-Auto Mr-Auto marked this pull request as ready for review November 1, 2023 20:13
@Dregu Dregu merged commit a2b3641 into spelunky-fyi:main Nov 2, 2023
10 checks passed
@Mr-Auto Mr-Auto deleted the stuff branch November 26, 2023 09:44
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.

2 participants