I don’t review code changes on drop box. Could you read the Guide for New Contributors and submit your changes to github so I can get them merged?
I hadn’t expected any interest from the core team, and so intended to let @Turn478 (the only person who’s shown any visible interest so far, apart from someone who wanted the code to work differently) to have a look at it before I submitted a pull request, at which time I thought the core team might take an interest . Once github gets involved everything becomes a cumbersome, error prone mess, so I’d wanted to have to change as little as possible after that point.
Thanks for the ping, I forget to check this forum regularly. Having mlangsdorf review the changes would definitely be helpful. I hadn’t considered trying to keep both cores, only the expansions since those can easily allow multiples. Multiple cores would be fantastic.
I went ahead and tried to take a look. The changes loaded ok, the hub core let me select between the 2 versions, but the expansions did not let me choose between versions. It all seems very promising and worth putting up on github. Please note that the older core has changed since 0.E-2 so you’d actually want to make sure you keep that version (more wall types, a few fixes in it).
my github id is curstwist if you see feedback once it’s posted there.
EDIT: ok I see how you did the selection of expansion versions. That’s a little confusing. I expected an intermediary menu like the core offered.
Also, I eventually hope we can add little ascii graphics of blueprints to the selection menu for players to have context in what they are choosing.
Thanks for looking at it. The older blueprints aren’t actually from 0.E2 stable, but from a newer experimental that I switched to for testing against a newer baseline.
The problem with the top level expansion menu is that it gets cluttered fast if versions are put there, especially since there aren’t any “full” descriptions at that level, so everything has to be crammed into what really should be the title.
Code wise, the addition of an additional full description would help. ASCII graphics would definitely be useful, but it would also be error prone as it would have to be done manually, and thus kept up to date manually. Variants would also be hard to describe, but it would be quite handy with a preview.
If I’d added the expansions in the same way as the bases you’d end up with a list something like this:
- Canteen version 1
- Livestock version 2
- Saltworks
- Workshop version 2
- Canteen version 2
- Garage version 2
- Garage version 1
- Farming Area version 1
- Storage version 1
- Farming Area version 2
- Storage version 2
- Workshop version 1
- Livestock version 1
since the entries in the menus get scrambled randomly.
I’m fairly happy with getting multiple cores working as it provides a framework for the development of future versions.
We’ll see what mlangsdorf says once I manage to get it to the infernal github. Thanks for providing your github handle, by the way.
Sure, This is all very worthwhile to add to vanilla and I really appreciate having another person to work with on faction camps. I know the UI needs a lot of work, maybe having these new variations will inspire others to take on the menu overhaul. I’m building out a few of these to see the actual differences between versions. Including screenshots of your new finished maps on the PR would be good.
I’m open to coming up with a format for menu titles/names of steps so they are uniform and offer as much information as possible.
I’m actually having a go at using that 3OMT garage for a base. I need to make some slight adjustments before making the maps which I’ve started working out today.
And finally. The older core should probably get the looser build restrictions for expansions like your core does. I can take care of that if this gets approved. In general, we shouldn’t have different restrictions on similar builds (field hubs), (all shelters), etc.
I did actually make a PR for a correction of the base recipe UI, but it’s stalled because the appveyor and travis tests failed on completely unrelated issues.
whats the PR #? I can’t merge but I can look at it and review it to help get it going.
looks like 45399. It isn’t that old so should be fine.
Correct. You found it faster than I managed to write an answer.
The next step is to somehow get the JSON linter thingy to sort of install. I don’t have high hopes after fighting with the astyle one…
you can use the browser version: http://dev.narc.ro/cataclysm/format.html
It will be very painful for 500 files.
You’ll need to break that PR up into manageable chunks as well for review it sounds like. Probably per expansion/OMT.
I don’t think that will work, based on past bad experience with github. The problem is that it won’t actually work in a piecemeal fashion, as it’s all dependent on the framework introduced in the first one, which will clash with everything that hasn’t been converted. The only reason github wouldn’t throw a fit is that it might not understand JSON, but I’m sure the tools used will complain.
It’s not even possible to separate “base” from “expansion” because part of the work is the splitting up of files into chunks to untangle the connections. The only reason it might somehow work anyway is because the game doesn’t detect multiple definitions of the same recipe, so it won’t complain about that, but which copy it will actually try to use is effectively random, I think.
Anyway, I haven’t yet given up on getting the JSON lint guff sort of working.
Edit:
I finally gotten the VS integration to “work”, i.e. the style-json.ps1 script to run after changing the name of the compiled JsonFormatter-vcpkg-static-Release-x64.exe to Json_Formatter.exe that the script accepts (and managing to find that the script prohibiting policy had to be changed for the user, not the machine to take effect). That was spectacularly useless as it just rejected more or less every single file with
“Needs linting : .\data\json\mapgen\basecamps\base\modular_hub\modular_field_common.json
Please read doc/JSON_STYLE.md”.
I’d already expected everything to be rejected as the files included in CDDA, at least the 0.E2 stable ones I started from, didn’t seem to meet the formatting mandates mentioned in whatever document I read first (and only seen in JSON_STYLE.md if you read the sentences very carefully and understand that “object entries” means curly brackets at any level), namely lots of entries with curly brackets on the same line, but I had hoped that it would at least do an astyle reshuffling of the files, or, at a absolute minimum, point out what it didn’t accept, not just “Barf! Change to satisfy my demands!”).
This is going to be a long and painful process of eternal copy-paste-copy back (on the optimistic assumption that the web thingy even works, rather than being another version of “fix it to my satisfaction”).
Edit 2: It seems the json-formatter thing is broken in another way as well: trying to use it on the current version of the files (rather than the old ones I had from some earlier testing in git connected version of CDDA) fails a lot with “Supply a filename to style or no arguments.”, which I suspect is caused by the deeper directory structure.
Edit 3: Modifying json_formatter to output its parameters on failure shows where it blows up: the blasted thing can’t handle spaces in folder names, and so thinks it’s being fed multiple parameters. I guess I have to uglify the folder names, then.
Edit 4: The modification above also shows that the “…\json\npcs\holdouts\Rural - Cowboy *.json” files are rejected.
Edit 5: Apparently json_formatter DOES modify the files: it just produces a horribly misleading “error” message. That, at least, saves me from a whole lot of pain.
Edit 6: This has finally resulted in a pull request: Base camp expansion #45560.
Glad to see your 1st PR up!
This bug doesn’t fall into the first PR so I’m putting it here. When I was building out the two canteens to check their progression and such, the legacy canteen allows me to build over the dining room area with the stables expansion buildings. I haven’t had time to track down where the error occurs but thought I’d mention it before that segment gets PR’ed.
I like what you’ve done with the new canteen as well : )
Thanks for the report. I’ve been severed from the net for a couple of days (someone dug where the cable was, apparently). I encountered the same error yesterday, and the error is in the stable recipe, which, for some strange reason, relies on the wrong “starting” identifier (and I believe that error is in the PR: I’ve got some sorting out do do).
As an aside, I’ve managed to sort the expansion list, so I’m about to issue a PR for that. If approved, that should allow for the expansion version management to be moved to the top level where it belongs.