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

Create timer and timerGroup struct #698

Merged
merged 5 commits into from
Jun 13, 2024
Merged

Create timer and timerGroup struct #698

merged 5 commits into from
Jun 13, 2024

Conversation

blmarket
Copy link
Contributor

@blmarket blmarket commented Jun 7, 2024

Motivation

To make the conversion easier, need to have a better understanding of undefined structs in the C code where it's hard to understand when the code has lots of pointer offset access. I found those 2 structs have a minimal external dependency (only need to use platform ticks) so created this change.

Changes

Made assumption of structs from the C code.

To avoid any possible regression, only the function parameter type has been updated. Some go unit tests were introduced to demonstrate the fields are working as expected.

Required sign-off

  • I confirm that my PR does not contain any commercial or protected assets and/or source code.
  • I agree in advance that my codes will be licensed automatically under the Apache License or similar BSD/MIT-like
    open source licenses in case if OpenNox Project will adopt such a non-GPL license in the future.

Made assumption of structs from the C code.

To avoid any possible regression, only the function parameter type has
been updated. Some go unit tests were introduced to demonstrate the
fields are working as expected.
@dennwc dennwc self-requested a review June 7, 2024 10:48
Copy link
Collaborator

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Great work! And you have my respect for adding the tests for the C code! 🙇‍♂️

src/legacy/timer.go Show resolved Hide resolved
src/legacy/timer.go Outdated Show resolved Hide resolved
src/legacy/timer.go Outdated Show resolved Hide resolved
src/legacy/timer.go Outdated Show resolved Hide resolved
src/legacy/legacy_test/timer_test.go Show resolved Hide resolved
src/legacy/legacy_test/timer_test.go Outdated Show resolved Hide resolved
src/legacy/legacy_test/timer_test.go Show resolved Hide resolved
src/legacy/audio_ail.go Outdated Show resolved Hide resolved
src/legacy/timer.go Outdated Show resolved Hide resolved
@@ -4193,7 +4193,8 @@ int nox_xxx_tile_486060() {
}

//----- (004862E0) --------------------------------------------------------
int sub_4862E0(int a3, int a4) {
int sub_4862E0(timer* a3p, int a4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for being careful and doing it one step at a time!

I should really give you an intro to the tooling I have in cxgo branch. It will make this process a lot faster.

The main idea is that you can discover C types there, and once you do that, it can automatically rewrite code based on the new field offsets.

Since it already has all the code translated from C syntax to Go, you look directly at that Go code in legacy there and compare to C only if in doubt. The way I usually work there is find a function of interest (let's say the one you have here sub_4862E0), follow the code looking for clues about the struct size (as you already did here). If you can discover struct layout immediately - that's awesome! If not, you make a temp Go struct in that code base:

type timer struct {
   field_0 uint32 // 0, 0
   field_4 uint32 // 1, 4
   field_8 uint32 // 2, 8
   field_12 uint32 // 3, 12
   field_16 uint32 // 4, 16
   field_20 uint32 // 5, 20
   field_24 uint32 // 6, 24
   field_28 uint32 // 7, 28
}

I have a tiny tool that generates these temp structs given a size. Can push it as well if you want.

So having either this, or your guessed struct layout, you change that Go function signature and run fixlegacy:

cd src
go run ./internal/fixlegacy

This tools doesn't have any dependencies and it will read all Go in legacy, will notice that you added a specific type to the function argument and it will automatically rewrite pointer offsets in that function to direct field accesses.

Notice that in my temp struct I didn't guess the type of last two fields, which should be uint64_t. This is fine. Tool is careful enough to not convert an offset access if it notices that access is done with a larger type than the current field size is. So you just guess and convert. If you see that some offsets remain - you adjust the struct field types and run the tool again.

With that iterative approach, you can quickly propagate your new types around, validating your struct layout. Once you're done, you will likely have clean Go code in this cxgo branch than can directly replace old C code.

Next steps are up to you. The safer path is to take only structs first, as you did in this PR, and than slowly bring these clean new functions from cxgo to dev. But I'm not always patient enough, so I could move all code related to certain struct in one go (if I'm more or less certain that code makes sense).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, yeah that would help a lot! Surely the next step will be moving the existing C code to Go version, and I agree with the approach you mentioned. Let me try the tool to see it works with new structs.

blmarket added a commit to blmarket/opennox-lib that referenced this pull request Jun 8, 2024
It allows an unit test to control clocks.

For usages, see noxworld-dev/opennox#698
blmarket added a commit to blmarket/opennox-lib that referenced this pull request Jun 8, 2024
It allows an unit test to control clocks.

For usages, see noxworld-dev/opennox#698
src/legacy/timer.go Outdated Show resolved Hide resolved
src/legacy/audio_ail.go Outdated Show resolved Hide resolved
src/audio_ail.go Show resolved Hide resolved
src/legacy/timer.go Outdated Show resolved Hide resolved
dennwc added a commit to noxworld-dev/opennox-lib that referenced this pull request Jun 10, 2024
* Add MockPlatform which mocks Platform interface

It allows an unit test to control clocks.

For usages, see noxworld-dev/opennox#698

---------

Co-authored-by: Denys Smirnov <dennwc@protonmail.com>
@dennwc dennwc merged commit 4d31a78 into noxworld-dev:dev Jun 13, 2024
8 checks passed
@blmarket blmarket deleted the timer branch June 14, 2024 02:34
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