MaKiPL / OpenVIII-monogame

Open source Final Fantasy VIII engine implementation in C# working on Windows and Linux (Android and iOS planned too!) [Monogame]
MIT License
637 stars 58 forks source link

Coding Conventions / Architecture #98

Closed benjaminfoo closed 5 years ago

benjaminfoo commented 5 years ago

I think some parts of the project need some clarification on how the code should be structured (using camel-case for the code, some kind of formatting, comments, amount of magic numbers, etc.), for example - the file module_world_debug.cs should be called Module_World_Debug.cs or ModuleWorldDebug.cs. There is also a lot of static code used where an OOP approach maybe would be a better fit.

Another thing which I noted is the low amount of documentation (meaning and uses of classes as a header-comment, meaning of members in classes, method-comments, etc.) - this should be refactored in order to lower the amount of work for new people within the project.

Sebanisu commented 5 years ago

Sorry for the mess heh. The menu module ballooned as I started adding new screens. I was going to go back over it when I when I had all the screens done. I am kinda a learn as I go kinda person. I had some done some coding but not at this scale.

benjaminfoo commented 5 years ago

No reason to apologize :) - sometimes the urge to create progress in a project like this is so top prio, that everything else simply doesnt matter, or anything else is just more important. However, I think the amount of technical debt should be kept at a low level and the things I mentioned are, at this level, still very manageable.

Here is a list of stuff that needs to be done / talked about:

I'd be happy to help you with this :)

MaKiPL commented 5 years ago

Ad.1+2 - naming conventions are not followed- at least not in the older classes. They should be changed Ad.3 - in many situations this is intentional. The modules can be changed in real-time meaning they have to store some variables even when they are not used. Example is changing the module from field to battle or from field to cardgame and back. Even though that the module is no longer used when example in card module, then as soon as the module ends it gets back to the same state of field it was just before jumping into the cards. It's the same behaviour as in vanilla. Some modules even before init can talk or query variables of static properties of others. OOP is mainly used for creating objects that are mostly created and destroyed afterward like enemies on the battle module. Ad.4 I'll be working on it Ad.5 Sure, good point Ad.6 Yeah, the same Vertex/Vec3 struct for example is copied over at least three different classes- bad design

Sebanisu commented 5 years ago

Ad.6 I've seen this where like structs exist in XNA but we had a custom struct for it. In Tim2.cs and Tex.cs for color, so i changed it to work with the XNA color struct. Might be other examples of this. Because the classes were ported from projects without xna.

benjaminfoo commented 5 years ago

I think we should introduce something like a vector2 and vector3 class for the 6th point and use this instead, could be beneficial for caching.

Sebanisu commented 5 years ago

XNA has a vector2 and vector3 struct https://docs.microsoft.com/en-us/previous-versions/windows/silverlight/dotnet-windows-silverlight/bb199660(v%3Dxnagamestudio.35) https://docs.microsoft.com/en-us/previous-versions/windows/silverlight/dotnet-windows-silverlight/bb199670(v=xnagamestudio.35) Do you mean we shouldn't use those?

benjaminfoo commented 5 years ago

No, we should use these (from the framework) if they are already implemented.

However, there are quite some classes which contain more than two or three members (Normal and Vertex) for example, which could be replaced by a quaternion or vector4. Some of these classes (polygon) for example are an aggregation of multiple those classes (public byte F1, F2, F3, N1, N2, N3, U1, V1, U2, V2, U3, V3, TPage_clut, groundtype, TextureSwitch, unk2;) where this could be done with three vector3 values, so much possibilities though.

benjaminfoo commented 5 years ago

As all of the involved people within the project were interested in an architecture, i'd like to close this issue and create a new one with ideas / concepts for an alternative