search

calref / cboe

Classic Blades of Exile
http://spiderwebforums.ipbhost.com/index.php?/forum/12-blades-of-exile/
Other
167 stars 41 forks source link

Redo2 #307

Closed fosnola closed 1 year ago

fosnola commented 1 year ago

So a new pull request to replace https://github.com/calref/cboe/pull/285 ( after doing a rebase with master on 01/11/23)

CelticMinstrel commented 1 year ago

Please delete the first commit (my commit) and force-push.

fosnola commented 1 year ago

Please delete the first commit (my commit) and force-push.

This one: {deleted} ?

Note: Ok, normally, I just do that.

CelticMinstrel commented 1 year ago

Okay, I'm going to slowly work my way through these commits, and in some cases I will open up a comment thread on a specific commit if I think it looks wrong or otherwise require additional explanation as to its purpose.

CelticMinstrel commented 1 year ago

General note: for the asan commits I'm going to be investigating and doing my own thing, which may in some cases be different from what you did. So, when you next rebase this PR, the asan commits will probably conflict, but in most cases you can just skip them. (Do double-check to make sure I didn't miss something though.)

fosnola commented 1 year ago

For your information, in Xcode, you can easily add an asan check by using the menu Product, Scheme, Edit Scheme... and then clicking on Run Debug, Diagnostics, Run Sanitization, Address Sanitizer...

CelticMinstrel commented 1 year ago

Yes, I'm aware, thank you. I actually did that a little while ago to fix a crash in the vector2d resize unit test.

CelticMinstrel commented 1 year ago

Okay, I got roughly as far as cc8b9be4fee667e63376a5f1323d61ee3574c77c, at which point I start running into a lot of conflicts. It's likely because I skipped the textures commit.

It would be nice if you could rebase on the latest master again and resolve the conflicts. Many of the commits would probably be dropped automatically, and there are also several that have been addressed in other ways which should also be dropped. I commented on some of them in this thread, but I may not have commented on all of them. In particular, if it's an asan commit, it's quite likely I already did something else to address the issue.

I'm going to close this now. We can continue to discuss the specific commits that have open threads on them in here, and once you've rebased your work you can open a new pull request.

fosnola commented 1 year ago

It would be nice if you could rebase on the latest master again and resolve the conflicts. Many of the commits would probably be dropped automatically, and there are also several that have been addressed in other ways which should also be dropped. I commented on some of them in this thread, but I may not have commented on all of them. In particular, if it's an asan commit, it's quite likely I already did something else to address the issue.

I will try to rebase it next week ( but it may take some times to do that ) and I will try to answer tomorrow the two points I left unanswered...