Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

☂️ Game engine: Issues with general code organisation #3200

Open
7 tasks
Tracked by #3143
olanti-p opened this issue Sep 21, 2023 · 3 comments
Open
7 tasks
Tracked by #3143

☂️ Game engine: Issues with general code organisation #3200

olanti-p opened this issue Sep 21, 2023 · 3 comments
Labels
easy Simple task that doesn't require understanding the code much ☂️ umbrella tracks multiple issues at once

Comments

@olanti-p
Copy link
Contributor

olanti-p commented Sep 21, 2023

We have some issues with general organisation of code:

  • There are many "god objects" in the code. The most infamous are game, map, item, Creature and all its children: monster, Character, player, npc, avatar. Besides the standard problems of "god objects", these classes also form some of the biggest and most included headers we have, which negatively affects compilation times for both clean and incremental builds. Ideally these classes should be broken up, or stripped of methods that can be converted into free functions (especially those methods that only operate on other public methods and those that don't touch the object at all). Some effort has been done on this front, but nothing substantial.
  • We try to preserve code compatibility with DDA to make porting updates from there easier, but it's causing a massive block on some tooling (style: simplify formatting with deno, part 2: JSON #3118) and code organisation ([WIP][Do not merge yet] Refactor #236) changes. With 3.5 years worth of divergence, we may want to reconsider this stance, as most attempts at porting end up with conflicts anyway.
  • All source files exist in a single folder src, with no clean separation for frontend/backend, or purpose, or library they belong to. Moving everything to subdirectories may not be desirable since that'd ruin compatibility with DDA (unless we decide we don't care about that), but some parts at least could be moved out of the way.
  • Some functionality is spread out in code in unintuitive ways. For example, output.h header contains a mix of string formatting code, generic UI output code, item-specific UI code, UI management code and UI widgets, but cursesdef.h also contains UI code, as well as cursesport.h and ui.h, but ui.h also has widgets and helpers, and there's also ui_manager.h that has UI management code, and various other headers such as calendar.h can have their own formatting functions.
  • Namespaces are underutilized: many globally available functions and constants, mainly ui code, are not sorted out into namespaces while they could be.
  • We have Code Guidelines and Best Practices for C++ code, but neither are enforced for PRs or even hold true for existing code.
  • We have inherited large code migration projects, those will have to be finished eventually: Vestigial code oddities #318 (comment) Moved to ☂️ Game engine: Ongoing code migration projects #3271
@olanti-p olanti-p added the easy Simple task that doesn't require understanding the code much label Sep 21, 2023
@YakumoChen
Copy link
Contributor

We have inherited large code migration projects, those will have to be finished eventually: #318

Cute to see my 2 year old triple digit issue report mentioned

"Finish eventually" is a little loaded, half the issues I made were caused by temperature being disabled, the other half could have easily been solved by selective reverts in the code, undoing those projects to before they were started. I'd be shocked if those are still in the game, honestly, it's just a revert.

@olanti-p
Copy link
Contributor Author

olanti-p commented Sep 26, 2023

"Finish eventually" was specifically about the issues listed in the comment #318 (comment) (GitHub UI inserts the issue title, which makes it a bit unclear that one of the comments was linked). Out of those projects, there's no sense in reverting them as they're straight up upgrades over existing architecture. I've moved that comment to #3271 for clarity.

Regarding #318 itself, I've checked and it seems we actually did handle all the issues originally listed there, so I've closed it as completed. If anything new pops up (or old stuff reappears), feel free to open new issue.

@scarf005
Copy link
Member

but it's causing a massive block on some tooling

i made a JSON query/manipulation API for BN (https://github.com/scarf005/catjazz) and working around this has been painful.

https://github.com/scarf005/catjazz/blob/main/utils/json_fmt.ts

@scarf005 scarf005 added this to Roadmap Jan 7, 2024
@github-project-automation github-project-automation bot moved this to Todo in Roadmap Jan 7, 2024
@scarf005 scarf005 changed the title Game engine: Issues with general code organisation ☂️ Game engine: Issues with general code organisation Mar 22, 2024
@scarf005 scarf005 added the ☂️ umbrella tracks multiple issues at once label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy Simple task that doesn't require understanding the code much ☂️ umbrella tracks multiple issues at once
Projects
Status: Todo
Development

No branches or pull requests

3 participants