search

ko4life-net / ko

Open source development of the game Knight Online. This is a reversed engineered old version of the game aiming to replicate the nostalgic experience we all once had <3
https://github.com/ko4life-net/ko
MIT License
55 stars 22 forks source link

Implement ExitMenu v1264 UIF. #166

Closed xGuTeK closed 1 year ago

xGuTeK commented 2 years ago

https://github.com/ko4life-net/ko/issues/136

Thanks to @stevewgr for RE code :)

stevewgr commented 2 years ago

Hi @xGuTeK, just so I don't feel bad about the many comments, I cleaned up at least the ones that I think there is not much room for discussion and to save you the efforts. The rest I left opened in case you have anything to add. Let me know if you need any help.

stevewgr commented 2 years ago

Hi @xGuTeK, could we please not resolve comments unless they were addressed with either a link to a commit with the change or conclusion of whether we both agree to skip or address it differently. This makes it very difficult for me as a reviewer to keep track of the current state of this PR. Also I see the last commit from you completely broke encoding of the files: https://github.com/ko4life-net/ko/pull/166/commits/9e42dbf6dd09bd58e67cf995c3d04d520e07efd4

Could you please check? I unresolved all the comments you resolved without properly addressing them.

stevewgr commented 1 year ago

@xGuTeK is busy atm, so to help out I rebased the PR with master and also applied some of the feedback. There are few other issues that needs to be fixed which I noticed when selecting character:

There are some issues that I'd rather we work them out before merging this big and import change.

stevewgr commented 1 year ago

I fixed the second issue here: https://github.com/ko4life-net/ko/pull/166/commits/6799f3d73452fc49bf88a81a5d73ad7df3e55e59

Fun fact, it was also noticed during the code-review:

stevewgr commented 1 year ago

Rebased with master and build passing: https://github.com/ko4life-net/ko/actions/runs/3977641142/jobs/6818925638

Merging. 🚀