Decoupling Xe.Tools from OpenKH

[TopazTK]

This needed to happen. The inclusion of the Xe.Tools library has been a sore point in OpenKH that made a lot of the contributors uncomfortable ever since the project has been introduced to the public. This PR aims to remove Xe.Tools entirely and replace it with pure C# alternatives.

Note that in it's current state this PR is nowhere near ready to merge. It needs a lot, and I mean, a lot of testing and joint effort. This is basically a complete overhaul.

Should anyone want to contribute to this, please do so. Because this is a monumental project that I cannot do alone.

Complete:

• N/A

Complete, but needs testing:

• OpenKh.Common

Work in Progess:

• N/A

[Rikux3]

Interesting to see you contributing, as a few days ago you've said you were 'done contributing anything here'.

Before i talk about the code:

• Why exactly does it need to happen? Was this ever broad up or discussed with any other contributor/developer beforehand? Of course PRs/Drafts can also be a place to start such conversation, i'm just confused about the timing.

• I don't really understand the pressure to remove the Xe.Tools suite at this time. As you may have seen, it was never enforced to use it, neither was BinaryMapper. There are countless classes which use the "traditional" BinaryReader procedure. To me this seems more a bit like 'The person who wrote this dependency left the project so now it's time to kick it out'.

• You also say the inclusion of Xe.Tools made 'contributors uncomfortable'. To what extend? Of course i've noticed that some people had difficulty understanding the code base, but i'm not sure if that applies to Xe.Tools. Kenjiuno even made an PR to the WPF components.

Now, the code:

• The code you've posted for a "pure" mapping function isn't working at all. I've compiled a little .net fiddle which you can find here. It seems that Marshal.SizeOf doesn't work with classes, but only with structs. I'm also worried that you're using unmanaged structures.

• Another issue i have is that classes that were pure "data" classes (with properties only to describe a certain structure, like HdAsset) now need a constructor with the ugly BinaryReader shenanigans. This is boilerplate code that just inflates the file.

• If a class (or a struct) has additional members (this file would be a good example, the properties FormId, FormLevel etc.) will these members also get "serialized"? If so, this would be a major flaw. With the current implementation, you circumvent that with marking the properties which should be serialized. (I know i'm using the term wrong but i don't know a better one).

• Why in god's name do you remove a dependency (Xe.BinaryMapper) from a base project (OpenKh.Common) that is used in almost all the other projects? You can't even build the solution anymore. You can't even test your changes with the unit tests that the solution offers. There have to be smarter ways to approach that. I'm really questioning how you could say that OpenKh.Common is 'Complete, but needs testing'. Nothing is complete without testing.

Overall, i'm not sure why you want to enforce this drastic change now. The project is struggeling anyway (and i personally don't think it's because of Xe.Tools or Xe.BinaryMapper), why can't people implement it one or the other way? KeyToTruth did that with most of the BBS tools, he used WinForms and BinaryReader because he was more comfortable with it (at least from what i know). I'm not really sure why removed that base dependency. How would someone even pick up this draft and work on it? Would you move all the testing at the end, with a bucket-load of errors?

[TopazTK]

Why exactly does it need to happen? Was this ever broad up or discussed with any other contributor/developer beforehand? Of course PRs/Drafts can also be a place to start such conversation, i'm just confused about the timing.

It was. Ever since OpenKH was publicized, Xe.Tools was the point of argument between Xeeynamo and almost every other contributor.

I don't really understand the pressure to remove the Xe.Tools suite at this time. As you may have seen, it was never enforced to use it, neither was BinaryMapper. There are countless classes which use the "traditional" BinaryReader procedure. To me this seems more a bit like 'The person who wrote this dependency left the project so now it's time to kick it out'.

It was enforced a lot to be honest. And even wen the person was in the project this toolkit was heavily disliked and there were countless dramas on it.

You also say the inclusion of Xe.Tools made 'contributors uncomfortable'. To what extend? Of course i've noticed that some people had difficulty understanding the code base, but i'm not sure if that applies to Xe.Tools. Kenjiuno even made an PR to the WPF components.

With the words of another collaborator when the removal of Xe.Tools was brought up again: "Yes. God, fucking yes. I begged Xeey to do this so that his name isn't slapped everywhere all the time while nobody else has the same treatment."

The code you've posted for a "pure" mapping function isn't working at all. I've compiled a little .net fiddle which you can find here. It seems that Marshal.SizeOf doesn't work with classes, but only with structs. I'm also worried that you're using unmanaged structures.

I figured it may happen, this is why help is wanted and this is a draft. I pulled these codes from my projects with structs thinking it may work with classes, apparently not!

Another issue i have is that classes that were pure "data" classes (with properties only to describe a certain structure, like HdAsset) now need a constructor with the ugly BinaryReader shenanigans. This is boilerplate code that just inflates the file.

If it has to happen it will happen. We can organize it though.,

Why in god's name do you remove a dependency (Xe.BinaryMapper) from a base project (OpenKh.Common) that is used in almost all the other projects? You can't even build the solution anymore. You can't even test your changes with the unit tests that the solution offers. There have to be smarter ways to approach that. I'm really questioning how you could say that OpenKh.Common is 'Complete, but needs testing'. Nothing is complete without testing.

In all fairness, OpenKh.Common may have been a bad place to start.

Overall, i'm not sure why you want to enforce this drastic change now. The project is struggeling anyway (and i personally don't think it's because of Xe.Tools or Xe.BinaryMapper), why can't people implement it one or the other way? KeyToTruth did that with most of the BBS tools, he used WinForms and BinaryReader because he was more comfortable with it (at least from what i know).

I am not enforcing anything at any time, do not get me wrong. This is a draft and nobody said anything about it being urgent. But it needs to happen and it seems like only you are against it.

I'm not really sure why removed that base dependency. How would someone even pick up this draft and work on it? Would you move all the testing at the end, with a bucket-load of errors?

In all fairness I should have removed the code and kept the dep. Will work on it further.

[Delta-473]

Eeuhm the binary mappers is pretty easy to use and can deal with big endian (which is important for PS3), I understand that you want to remove the amount of dependencies. We can roll our own implementation that has feature parity with only what we need. But that is gonna require time. If you want to remove a part of dependency on Xe components I would recommend looking at the WPF parts but as I would like to replace WPF with something more Cross Platform that is just double the amount of work. However out of some projects Xe.tools can be removed already without to much work (An example of this is in OpenKh.Tools.Common Only the Kh2WorldsList uses a type from that) . Imho there are more important tasks to do (like reorganising files into directories to make things easier to search for) then replacing the code that reads and writes files. Because that's just the very bottom layer once you have read the file you don't use Xe reader anymore.

[Vladabdf]

I think in addition to accounting for readability purposes (on which a handful of would-be developers have not contributed much or any due to the sometimes incomprehensible Xe.Tools base that's been embedded in the project (in addition, of course, to the lack of organization for the overall structure of code)), I think the removal of Xe.Tools would be helpful for potential legal reasons. Xeeynamo made it explicit he wants nothing to do with the project anymore. The outbursts paired with it have me worried about backlash coming back on the project and its current members. I'm not implying anything in particular, but simply pointing out there are other logical reasons to remove the dependency that OpenKH has on Xe.Tools. It will not be a quick process, and certainly not easy, but there are benefits to doing so.

[Delta-473]

For the legal reasons the Xe tools are published under MIT license and it's indeed gonna require some planning to see what's easy to remove and to work from top to bottom. (ex starting with KH1 first because that's the smallest one to experiment what's a good design)

[Rikux3]

I've noticed just now that there seems to a mixup about what Xe.Tools actually contains: Xe.BinaryMapper is an completely independent piece that is responsible for mapping files to data and vice-versa. Xe.Tools is mostly stuff related to either GUI or Drawing stuff to the GPU.

If we talk about removing Xe.BinaryMapper, no one can convince me that it is a good idea and i'd never endorce it. It's just way to comfortable to use imo. Sure, if you want to scrub the name of the project, fork it into the organisation, modify it and use that instead. Xe.Tools takes a lot of boilerplate code and "hides" it. And i think that's why people are unhappy with it or don't understand what certain parts do. But Xe.Tools was also what jump-started the creation of GUI tools (mostly the simpler editors) because you could re-use so much code. I could start to write an editor in like 10-15 minutes. Finding an appropriate replacement for that will be very hard.

Also, very little nickpick:

This PR aims to remove Xe.Tools entirely and replace it with pure C# alternatives.

Xe.Tools and Xe.BinaryMapper are already pure C# :P

---

It was. Ever since OpenKH was publicized, Xe.Tools was the point of argument between Xeeynamo and almost every other contributor.

I can't find any arguments made on the Discord server (aside from occasional build problems reported). I know that Xeeynamo wanted to discuss the project mostly in public, so that wonders me.

It was enforced a lot to be honest. And even wen the person was in the project this toolkit was heavily disliked and there were countless dramas on it.

Again, same as above. Even if that was the case, the terms 'heavily disliked' and 'countless dramas' seem far-fetched to me. Tell me one person, aside from yourself, who 'heavily disliked' it. If you don't want in public DM me.

I think it was a mistake that the project was hosted on Xeeynamo's private profile for so long. Of course it started there, but it should've been moved into the organisation sooner. People attached the name and the project, possible ignoring the other contributors.

But it needs to happen and it seems like only you are against it.

I am against it because i don't see an improvement with this proposal. Only Delta and I (as devs) have voiced our opinions so far, so saying i'm the only one seems weird when the others are silent.

[gledson999]

The removal of Xe.Tools and Xe.BinaryMapper is useless if there is no any improvement, so there is no reason to remove it, I see it as a way to retaliate Xeeynamo and disqualify its work.

[Vladabdf]

The removal of Xe.Tools and Xe.BinaryMapper is useless if there is no any improvement, so there is no reason to remove it, I see it as a way to retaliate Xeeynamo and disqualify its work.

This is definitely not the case. I love Xeeynamo, and harbor no ill will against him whatsoever. I have been under the impression that his tooling gave him more importance than other developers since his name was slapped in extra places and nobody else's was. It's also confusing for newer developers I've heard, though I don't exactly actively talk to many people so I cannot say this with certainty and be genuine. I have, however, witnessed comments over the years that the code base is intimidatingly confusing and disorganized, and I even opened a pull request ages ago saying the same thing with a simple suggestion to attempt to fix it before it got as large as it is now.

[TopazTK]

The removal of Xe.Tools and Xe.BinaryMapper is useless if there is no any improvement, so there is no reason to remove it, I see it as a way to retaliate Xeeynamo and disqualify its work.

It could have been interpreted that way, yes. Only if I was not arguing with Xeeynamo himself over the use of Xe.Tools since before OpenKH was even public, just like literally everything else I am planning to PR :^)

[Vladabdf]

At this point I do not see this aspect coming to fruition. Multiple developers have stepped up who work with OpenKH as it currently is, with Xe tooling, and I have not read any complaints. I henceforth also formally retract my agreement to this task. Xe tooling is important and too integrated into the project to visbly rip out. As such, it will stay for the foreseeable future.