Load Economy Data from JSON#4944
Conversation
|
CC @Gliese852 @impaktor @fluffyfreak @The-EG - I'd appreciate your eyes on this PR, as it's been about five months in the oven and that's a long time to kick up bugs. |
|
Nice to see a big project coming to fruition! Now for the criticism ;)
So this would be stuff that's hooked into the economy, and not just the cargo hold (as is currently possible with custom cargo). Should not the commodity icon be set from json as well then? To me, it feels strange to have illegal in its own file, since what is illegal varies from system and faction, it's something set by the judicial system, rather than the economic system. Does it even serve a purpose to split the commodities into diffferent json files? I think common items could also be illegal at some place, like "Amish" banning computers & robots, or liberterian world allowing everything. I'd prefer if that line would be more blurred. I.e. drugs could go into agriculture (or a future tourist economy?), and guns into industrial. It would be good to have @ecraven and @laarmen (Now comming full circle!) pitch in with thoughts on this PR. |
|
My first thought from a user perspective : JSON is a PITA to hand-edit (no comments, quotes everywhere, no trailing comma, mandatory uneasy-to-reach (on my layout) glyphs ( And yes, despite this, this is an improvement over the n-e situation for non-developers contributions, as was n-e over the previous, full-C++ implementation. Yay! I'll leave inline comments when looking at the code, but the overall PR description sounds good. |
| local commodities = Economy.GetCommodities() | ||
| local economies = Economy.GetEconomies() | ||
|
|
||
| local EquipType = _G.EquipType |
There was a problem hiding this comment.
Why is it necessary to use a global table here instead of simply requiring EquipType ?
There was a problem hiding this comment.
There's a dependency hell going on here where Commodity.lua needs access to EquipType, but the rest of Equipment.lua needs access to the commodity data, because everything is interlinked. The global assignment is just a quick hack - the solution is to add a "preregistration" mechanism to the package table so that circular dependencies aren't a problem. I've built one of those before and it makes a huge difference for complex lua projects.
I might merge the contents of Commodity.lua back into Equipment.lua, but this was the first step in separating commodities from equipment so I kinda want to keep it that way.
| } | ||
|
|
||
| -- TODO: don't create a separate EquipType for each commoditity, | ||
| -- instead store cargo separately from ship equipment |
There was a problem hiding this comment.
Ah, that old can of worms... I hadn't the patience of doing so back then as we had at least one prominent mod (Scout+) that would have had a lot of breakage over this. How is the mod situation nowadays ?
There was a problem hiding this comment.
In my eyes, Pioneer mods don't exist right now. I haven't seen a recent mod (other than maybe one personal hobby project) be announced for the entire time I've been contributing, and all of the mods I can find are at least 4 years old. I'd rather move forward and break old mods than stay in this limbo of massively inflated savefiles and awkward cargo handling. (No offense meant to you, the equipment system is a massive step up from what was previous! It's just... not a good fit for cargo.)
If someone wants to upgrade existing mods, absolutely more power to them and I'd be more than happy to support them, but until that time I'm not going to let old and inactive mods be an excuse for leaving things as the status quo.
| return equipmentPrice[self][e] | ||
| end | ||
| local mul = e:IsValidSlot("cargo") and ((100 + Game.system:GetCommodityBasePriceAlterations(e)) / 100) or 1 | ||
| local mul = e:IsValidSlot("cargo") and ((100 + Game.system:GetCommodityBasePriceAlterations(e.name)) / 100) or 1 |
There was a problem hiding this comment.
This comment is NOT to be addressed for this PR, and is something that could be solved in an unrelated, newcomer-friendly PR:
The name API for the equipment instances is misleading. I'd expect a name to be a string that is displayable on screen, whereas this value is more of an ID. It's just a small papercut in the many pitfalls of our codebase, but it's easily solvable.
There was a problem hiding this comment.
Absolutely! This is a 1am hack to get things working - I'm not sure about the purpose of the volatile sub-object, but I needed to be able to pass around commodity names (and this isn't meant to be the final iteration of commodities anyways, name just so happened to be an unused key to attach this datum to).
| 8000.0, // A0 Giant | ||
| 9000.0, // B0 Giant | ||
| 12000.0, // O5 Giant | ||
| 12000.0, // M0 Super Giant |
There was a problem hiding this comment.
On Github this looks unaligned with the above data. @impaktor didn't you use to have a trick to make the GH code view behave sensibly ?
There was a problem hiding this comment.
Github's tabstops are weird, they don't respect our editorconfig file. You can set your tab width to 4 IIRC and that fixes things.
It looks perfectly fine in a 4-TAB text editor, so I'm going to call it an acceptable weirdness.
There's no modding community anymore that I'm aware of, so I say we need not worry too much about that, until someone complains, in which case we can help them be pioneer anno 2020 compliant.
GH should use the definitions in .editorconfig |
I had the same thought, and it may be confusing since many items in the other files are commonly illegal, like animal meat. Also, probably to the same point, slaves are in both illegal and agricultural. I think it would be more intuitive to put everything into a category and let the system decide what is illegal and not. That being said, I'll try to get this compiled and run my precious metals<->air handlers route that I found and see how it compares! (among other things) |
|
@laarmen: I'd like to append to my answer: github cheat sheet says you can append |
|
So I do agree about the illegal file. It's definitely not the perfect way to do it, and I'd ideally like to make that the purview of a faction, where each faction can set commodity legality in their systems. However, I'm pretty much porting the existing code directly, and the existing code literally doesn't care about factions or anything else, and it definitely doesn't use the judicial system. It's just a hardcoded list of commodities in StarSystemGenerator. The illegals file and consumables file is actually a step up now, because you can tweak the chances (or consumption) without having to recompile the entire 1500 line generator. So until someone (probably myself) redesigns the economic generation code, the illegals file stays. Now the icon names. This is something I don't think is a problem - it's the UI's purview to map icons to commodities, not the other way round. The list of icon names is only there because I didn't feel like tracking down everything that used those icons and renaming them to exactly match the commodity names, but that's the eventual goal, to completely remove that list. Thank you all for the wonderful feedback so far, you've definitely given me some things to think about for the future! |
sturnclaw
force-pushed
the
json-commodities
branch
from
f997e81 to
38cf567
Compare
Going to have to go with 'not.' My old save loads without error, but as soon as I try to open the station window I get: It seems to work fine with a new game. I'll have to recreate my save with the console and otherwise play around a bit to see how it's actually working. That reminds me, I had the idea to write a script that would 'recreate' a save (basically put you in the same location, ship, money, etc)... |
Ooh, I'd be interested in that. |
|
And to add an additional comment about illegality - the |
Could YAML be an easier alternative to JSON? If so, imho this would apply to all data files, not just for the commodities. |
@danielrogowski I have considered using YAML and while it's attractive from a user's perspective; we're depending upon several features that depend on the JSON spec, including the ability to merge-patch JSON files which allows mods far more flexibility than before. I'll keep YAML on the back burner, but I don't have any serious plans to switch to it for mod files anytime soon. EDIT: Thank you for the suggestion though! It's something I've been considering but isn't really worth the work to switch at this point. |
sturnclaw
force-pushed
the
json-commodities
branch
from
38cf567 to
33d1800
Compare
|
Finally, I think it's close to done. Would appreciate as much help as possible debugging this; all of the hard work should be done, but just poke around and let me know if you find any issues, missing translations, etc. |
|
Looking into @The-EG's issue, it looks like old saves serialize equipment objects in their entirety so commodity items don't have the correct fields. At the moment, I'm going to mark this as a savegame bump, but I'll put some more thought into the issue and see if I can find a work-around. |
|
As an extra-fun change, now saved cargo items are collapsed to only save one table per cargo type per ship instead of 4,000+ (one for each item) for the larger bulk ships. This change will be cherry-picked into another PR but I'd like some testing first! 😄 |
sturnclaw
force-pushed
the
json-commodities
branch
from
701826f to
ddcd865
Compare
sturnclaw
force-pushed
the
json-commodities
branch
2 times, most recently
from
e7a0dda to
d5e6fc5
Compare
Serialize economy object when loading/saving. Use Log framework for Economy error messages
- Don't pass a cargo object anymore to GetCommodityBaseWhatever; just pass the commodity name - Lua doesn't use commodity ID anywhere, we should only need to serialize commodity name->ID mappings for C++ code - Add LuaMetaTypeGeneric for "class object" bindings. - Fix system short description being the l10n key
Update formatting in StarSystem.cpp Clean up gratuitous use of auto
Color commodity names based on profit Highlight profitable trades with a green profit icon, unprofitable (reverse) with red loss icon.
Moved to the SectorMap where it belongs for now Will reconsider location when economies are per-body/station
sturnclaw
force-pushed
the
json-commodities
branch
from
d5e6fc5 to
2679442
Compare
|
Finally removed the |
|
Since most of the code hasn't changed since the initial review (and I've had some smaller reviews on the Lua side) I'm planning on merging this later today. If you'd like to throw a sabot in the works, speak now or forever hold your peace! |
Factions which do not manually specify commodity illegal info inherit default illegality information Removed the illegals file and added default_legality to commodities (defaults to always legal) If a faction does specify a custom illegal commodity, all other commodities are assumed to be legal.
sturnclaw
force-pushed
the
json-commodities
branch
from
911231f to
699b39b
Compare
7 participants
Load commodity and economy data from JSON
This PR completely rewrites the existing Economy system to load all relevant data about commodities from JSON, enabling modders to extend the game with new commodities, different commodity generation weights, and eventually, entirely new economies![1]
[1]: this message not contractual and will take quite a bit of work to implement - our current system generation setup doesn't quite understand what an "economy" really is, and economies have pretty much no impact on commodity pricing. This is something for the future when individual markets have their own economy types and intra-system trade is possible.
Additionally, this removes quite a few lines of duplicated commodity data between Equipment.lua and Economy.h, which should result in this PR coming... somewhat close to breaking even in terms of lines changed.
(No, I don't believe that while I'm writing it, and neither should you.)
The real upside here is that a lot of previously-hardcoded information about system economies is more data-driven and far less complex to reason about on the C++ side of things - what used to be a 100-line list of commodities is now a one-line array and 200 lines of loader code. (Yes, I know, but it's way easier to use that array now, and the generation code is much better for the change.)
I've made an effort to find and fix as many bugs as possible and ensure it operates as close to "vanilla" behavior as possible, but I need more eyes on the ball before I'm comfortable merging this in.
I've left commit history relatively as it was written, and included impaktor's (slightly modified) commodity debug tab to ensure that system generation is functioning as intended.
I'd appreciate it if all interested parties could try this branch and just do a sanity check during normal gameplay - it may or may not be backwards compatible with old saves, but I'm going to assume it is until proven otherwise.
TODO:
StarSystemGenerator.cpp