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

Add mattb325:residential-houses and mattb325:residential-multi-unit-pack #41

Merged
merged 16 commits into from
Nov 21, 2024

Conversation

sebamarynissen
Copy link
Contributor

@sebamarynissen sebamarynissen commented Nov 15, 2024

This PR adds mattb325's entire set of 55 residentials to the channel.

The metadata has been generated with a script that detects the dependencies automatically. That was needed because the download on SC4Evermore just lists the BSC common dependencies pack which kind of defeats the purpose of using sc4pac as you don't need the entire bsc set for this pack, only a few.

Some images for fun:

image

image

image

The hotondo homes were apparently also hosted on Simtropolis and hence they have been included in the channel already. Removed them from this contribution.
@memo33
Copy link
Owner

memo33 commented Nov 15, 2024

Wow, that's amazing. Thanks. I had completely given up on those files.

How confident are you that the dependencies are correct and complete? Did you parse the lot config properties for relevant IDs?

Those early SC4E files were put together in a rush. While the DN file includes a couple of original Readme files listing dependencies, a lot of info seems to be missing. Probably your work here will help getting the info up on SC4E as well.

@sebamarynissen
Copy link
Contributor Author

I'm fairly confident that they are correct and complete. At my first try, I suffered from a lot of brown boxes. Then I dug a bit into the inner workings of SimCity 4 and found out that my script wasn't tracking the models. I learned that this was referenced by the resource key types, and tracking this down too I was able to get rid of the brown boxes.

Obviously it's harder to figure out whether all prop dependencies are there too because they simply don't show up instead of a brown box. The lots that I tested were looking fine, though I was not able to test and inspect all 55 of them in detail.

If you're interested in the dependency tracking code, you can find this here

I'm also working on the multi-unit set, though here my code still results in a few brown boxes. Maybe you could wait to merge this PR until I have figured out if something still needs to be changed. I'm away this weekend, but I plan to work on it further on Monday.

* Figure out dependencies for multi-units

* Update residential-multi-units.yaml

* Add climate variants to Casa de Manana

* Add entire pack

* Fix inclusion pattern for R$ terraces

* Fix discrepancy between MN and DN folder structure

* Fix name of l-aquila-apartments

* Fix potential conflict
@sebamarynissen sebamarynissen changed the title Add mattb325:residential-houses Add mattb325:residential-houses and mattb325:multi-unit-pack Nov 17, 2024
@sebamarynissen
Copy link
Contributor Author

I've added the multi-unit pack as well using the same strategy of manually tracking dependencies.

I've also ensured that my script finds every building, prop or texture referenced from any of the files (including from the SimCity_*.dat) and it is able to find everything. I've also done some more tests, and everything looks good to me.

@sebamarynissen sebamarynissen changed the title Add mattb325:residential-houses and mattb325:multi-unit-pack Add mattb325:residential-houses and mattb325:residential-multi-unit-pack Nov 17, 2024
@sebamarynissen
Copy link
Contributor Author

I noticed that a lot of the individual packages from the multi-unit pack could also be found on Simtropolis, so I referenced to Simtropolis and used the images from there where possible instead.

@noah-severyn
Copy link
Contributor

Obviously it's harder to figure out whether all prop dependencies are there too because they simply don't show up instead of a brown box. The lots that I tested were looking fine, though I was not able to test and inspect all 55 of them in detail.

If you're interested in the dependency tracking code, you can find this here

If you're interested, to find the prop and texture dependencies on a lot you could run the LotObject IIDs against the Prop Texture Catalog database, an sqlite db. It's a bit out of date as it hasn't been updated since the BSC Common Dependencies was released, but it might give you a good enough idea anyways. Updating it is on my todo list.

@memo33
Copy link
Owner

memo33 commented Nov 21, 2024

Ok, this looks good to me. Just the descriptions like

2 R$$ house models for small lots

are a bit unclear. It sounds like it might contain just models without lots. Also, "small lots" is not the same as low-density residential lots. Judging from your picture, some of the lots are not that small. Maybe something like "2 R$$ houses on low-density lots" might be clearer.

I'm also wondering whether including this info directly in the summary instead of the description would be more useful, as the summary is visible before opening a package page. The summary is also searchable in contrast to the description, but that doesn't matter that much here.

What do you think?

One more thing: I'd suggest to rename the collection packages to residential-houses-collection and residential-multi-unit-collection, similar to some of the existing collection packages, as that would make them easier to identify.

@sebamarynissen
Copy link
Contributor Author

Makes sense to go for "2 R$$ houses on low-density lots". I will change it. I didn't really know what to put in the description as there is not much information about the individual packages on SC4E, and I didn't really feel like writing a description manually for all these packages.

Not sure about the summary, I would personally keep it in the description, but let me know what you want.

I agree to rename them to collections as well, keeps them in line nicely indeed with the other collections already on the channel.

@memo33
Copy link
Owner

memo33 commented Nov 21, 2024

I didn't really know what to put in the description as there is not much information about the individual packages on SC4E, and I didn't really feel like writing a description manually for all these packages.

Ok, that's understandable.

Not sure about the summary, I would personally keep it in the description, but let me know what you want.

Alright, let's keep the summaries as is, then.

One more unrelated thought. The residential and commercial subfolders are filling up now, which is great to see. In the long run, it probably makes sense to split them up into smaller folders, but I'm not sure if dividing by wealth or dividing by density is preferable. Some files would be difficult to categorize. Even now some packages include residential and commercial buildings simultaneously.

@sebamarynissen
Copy link
Contributor Author

I made the changes we discussed. About splitting them up into smaller folders, I see why you would do that, but the difficulty is always that it's hard to choose a single parameter to categorize them on. Wealth and density are obvious candidates, but still I'm not sure if it's worth it. For example, in Aarom Graham's collection, lots often have two wealth types: CS$$ and CS$$$ for example, so in that case categorizing by wealth is hard.

@sebamarynissen
Copy link
Contributor Author

One additional thing about this: I noticed that there are sometimes small discrepancies between the MN and DN versions in the folder structure. For example, sometimes a folder name has no spaces (CamelCase) in the MN pack, but Camel Case in the DN pack. Would it make sense to use regexes for this as much as possible (Camel ?Case) so that if the pack is updated later on with a slightly different folder structure it doesn't suddenly break?

@memo33
Copy link
Owner

memo33 commented Nov 21, 2024

Would it make sense to use regexes for this as much as possible (Camel ?Case) so that if the pack is updated later on with a slightly different folder structure it doesn't suddenly break?

Yes, if you can anticipate the future name, it makes sense to do so. It can be hard to predict the name changes however. This is a general problem with this big packs on SC4E, as the folder names are changed frequently. For example, often additional version suffixes (v2, v3,…) slip in, or the name of a folder changes completely as it's part of the installer UI.

@sebamarynissen
Copy link
Contributor Author

I wonder if it would help if we start advocating for sc4pac more so that creators of packages know what the implications can be of updating their folder structure. In an ideal (not so far) future, sc4pac will be the default way of installing plugins anyway, and plugins should - in my opinion - be designed for this. For example, there is really no need anymore for those Java installers when using sc4pac (except for stuff like the NAM of course), so when sc4pac is widely used, I don't think installers should still be created.

@memo33
Copy link
Owner

memo33 commented Nov 21, 2024

Yeah, you're right. I hope once the GUI is ready, it's going to gain more traction. There may still be some resistance by people used to the old ways, but I guess modders would have an interest in making their mods easy to install.

Thanks a lot for the changes, as well. I'm going to merge the PR.

@sebamarynissen
Copy link
Contributor Author

If you give me a few minutes, I'll push a change with inclusion patterns that are better prepared for changes.

@memo33
Copy link
Owner

memo33 commented Nov 21, 2024

Of course.

@sebamarynissen
Copy link
Contributor Author

Allright, I've updated the inclusion patterns so they should be more robust if the folder name changes slightly in the future.

There's something else I noticed though, which is that the residential mulit-unit pack contains some shared resources - just like #46 and #47. Let me just ensure that those get included too.

@sebamarynissen
Copy link
Contributor Author

Looks like that file from the shared resources is nowhere referenced, so everything looks fine. Lgtm. 👍

@memo33
Copy link
Owner

memo33 commented Nov 21, 2024

Let's go.

@memo33 memo33 merged commit 6868df4 into memo33:main Nov 21, 2024
3 checks passed
@sebamarynissen sebamarynissen deleted the feature/mattb-residential branch November 21, 2024 21:45
sebamarynissen added a commit to sebamarynissen/sc4pac that referenced this pull request Nov 21, 2024
…t-pack` (memo33#41)

* Add `mattb325:residential-houses`

* Remove duplicate hotondo homes

The hotondo homes were apparently also hosted on Simtropolis and hence they have been included in the channel already. Removed them from this contribution.

* Add `mattb325:multi-unit-pack` (#3)

* Figure out dependencies for multi-units

* Update residential-multi-units.yaml

* Add climate variants to Casa de Manana

* Add entire pack

* Fix inclusion pattern for R$ terraces

* Fix discrepancy between MN and DN folder structure

* Fix name of l-aquila-apartments

* Fix potential conflict

* Fix naming convention

* Add pack images

* Add Simtropolis images where possible

* Link to Simtropolis where possible

* Scaffold industrial pack

* Revert "Scaffold industrial pack"

This reverts commit 5b80f32.

* Update residential-houses.yaml

* Rename residential-houses-collection

* Update name to collection

* Rename to residential-multi-unit-collection

* Remove catid from url

* Make inclusion patterns more robust

* Make inclusion patterns more robust
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants