Separate timer and dialog package out from legacy

noxworld-dev / opennox

OpenNox main repository.

GNU General Public License v3.0

440 stars 23 forks source link

Separate timer and dialog package out from legacy #706

Closed blmarket closed 2 days ago

blmarket commented 1 week ago

Hey, I'm not sure it aligns with overall direction you may want, so just created this PR to explain what I think. Feel free to give your opinion on it.

Motivation: Encapsulate dialog package so we can have unit test and integration test available for it without initiating whole legacy package.

Next steps:

Remove all memmap usages in dialog package
Make sure Dialog member fields are never accessed outside of the package
Instantiate Dialog instance

Required sign-off

[x] I confirm that my PR does not contain any commercial or protected assets and/or source code.
[x] 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.

blmarket commented 1 week ago

Seems now it goes a bit more wilder than I thought, but just giving a try.

Replace all C references with instance()

blmarket commented 4 days ago

Looks good to me!

Would you allow me to shuffle files around a bit? Since there's no C in tests now, the tests can be in the same package as the dialog, for example.

Also, I got an idea about the NewDialog. I was thinking maybe I can move all the function export variables like Sub_413890 and GetStringManager to a function argument of NewDialog. Less global state - the better.

Sure, sounds like a good idea. Seems it will be a big refactoring if we have to move all dependencies into a module.

dennwc commented 3 days ago

@blmarket Done! I moved all function to methods on Dialog and also removed the global state. Ready to merge, if all is good here.

blmarket commented 2 days ago

@blmarket Done! I moved all function to methods on Dialog and also removed the global state. Ready to merge, if all is good here.

Looks great to me as well! Thanks for the help! Will try to use similar structure for the future works. Passed all my manual in-game tests.

dennwc commented 2 days ago

@blmarket I had to squash all the commits, unfortunately. Initially I wanted to only group them (e.g. squash minor fixes only), but that Nox data file was in the middle of the commit log.