Skip to content

Conversation

@MonsterDruide1
Copy link
Contributor

@MonsterDruide1 MonsterDruide1 commented Jul 11, 2025

Required after merging open-ead/nnheaders#48


This change is Reviewable

@MonsterDruide1 MonsterDruide1 requested a review from leoetlino July 11, 2025 09:36
@MonsterDruide1 MonsterDruide1 self-assigned this Jul 11, 2025
@MonsterDruide1
Copy link
Contributor Author

Build checks obviously fail, because sead and nnheaders need to be updated at the same time. I manually checked that SMO compiles and this introduces no mismatches, no idea about BotW though.

Copy link
Contributor

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MonsterDruide1)


include/time/seadTickSpan.h line 18 at r1 (raw file):

    s64 toS64() const { return mSpan; }
    u64 toTicks() const { return mSpan; }

hmm, why did we have both toS64 and toTicks in the first place...? maybe we should just remove toTicks

@MonsterDruide1
Copy link
Contributor Author

I removed it in another commit - it requires two explicit casts, what do you think about it?

@leoetlino
Copy link
Contributor

idea: can we add a constructor to nn::os::Tick that takes a s64 (or u64, but I think s64 is more likely since we know that sead::TickSpan uses s64 all over the place)?

@MonsterDruide1
Copy link
Contributor Author

nn::os::Tick uses u64 internally. It makes sense that TickSpan is using s64, as it is the result of two Ticks being subtracted (=> can be positive or negative), but constructing a Tick from negative numbers does not make sense, so I think s64 is just wrong here.

Adding a nn::os::Tick::Tick(u64) allows the following replacement:
nn::os::ConvertToTimeSpan({static_cast<u64>(duration.toS64())})
=>
nn::os::ConvertToTimeSpan(duration.toS64())

@leoetlino
Copy link
Contributor

makes sense - u64 sounds like the better idea to me then

@MonsterDruide1
Copy link
Contributor Author

Updated accordingly.

@MonsterDruide1 MonsterDruide1 merged commit 9c5efc4 into open-ead:master Jul 15, 2025
1 of 4 checks passed
@MonsterDruide1 MonsterDruide1 deleted the nn-os-ticks branch July 15, 2025 21:12
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