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

Alternate implementation of my earlier town hall varient #54921

Merged

Conversation

JonathanLochridge
Copy link
Contributor

@JonathanLochridge JonathanLochridge commented Jan 30, 2022

LovamKiscsiGazsii Suggested an alternate way to add the same content I tried to add originally to another PR that wouldn't require changing any files other than the town hall file.
Ran through Linter already.

Co-Authored-By: Bence [email protected]

Summary

Content "Add Town Hall Varient"

Purpose of change

To add a new town hall variant to add variety.
Hopefully I should be able to add several others later.

Describe the solution

Added a new variant to the town hall with the following features
Changed the classroom on floor 2 to a set of conference rooms.
Made the windows more symmetrical.
Made the new varient use the same om_terrain codes so that it doesn't need new entries in other files.(This along with the original town hall design was done by Bence.)

Describe alternatives you've considered

Testing

Ran Changed file through linter.
Checked that it spawns correctly without errors on a fresh install of the newest experimental. Used debug menu to check several overmaps till I found a spawn of the new varient.
No errors messages occurred for me

Additional context

Completely fresh attempt at implementing #54554 I realized that it would be easier to make a new branch than try to revert each individual change in the secondary files.

Also, I spotted an unrelated map gen bug when I found a location to test at.
It's been a while since I saw roads dead-end into a turn like that. Not sure why it occurs or if it is already being worked on. Potentially I should make an issue for it since I didn't seem to see one?
I can provide a save for that if it would be useful.
Town Hall Sniper1 _2022-01-29T17-47-50

Another smaller but more relevant issue is that town halls tend to be prone to double spawns but seem to not occur in the majority of overmaps. There tends to be one every 6 or so overmaps. Which seems a bit low. Although, double spawns seem worse to me.

LovamKiscsiGazsii Suggested an alternate way to add the same content I tried to add originally to another PR that wouldn't require changing any files other than the town hall file.
Ran through Linter already.

Co-Authored-By: Bence <[email protected]>
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Jan 30, 2022
Not sure why the online linter didn't catch this? Just noticed it.
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 30, 2022
@Maleclypse Maleclypse added the Scenarios New Scenarios, balancing, bugs with scenarios label Jan 30, 2022
@Night-Pryanik
Copy link
Contributor

Are you absolutely sure your variant will merge seamlessly with the older variant? I mean, since you're using the same OMT names as in older variant, game will pick variants by random, which may lead to ugly edges.

@JonathanLochridge
Copy link
Contributor Author

I didn't consider that possibility. Although, I think due to having no difference in outer shape. It should be fine? I didn't see any bugs in the couple I came across while testing.

@Night-Pryanik
Copy link
Contributor

Just a friendly advice: don't rely on few encounters when testing. I personally debug-spawn locations about a dozen times to make sure there's no errors and everything is smooth.

@kevingranade kevingranade merged commit a1a0541 into CleverRaven:master Feb 5, 2022
@JonathanLochridge
Copy link
Contributor Author

Just a friendly advice: don't rely on few encounters when testing. I personally debug-spawn locations about a dozen times to make sure there's no errors and everything is smooth.

I usually test about 2 times for natural occurrences and like 3-5 times spawning it.
Which would put you at like 3-6 more tests after a confirmed working version? How often do you catch extra errors from that. I don't mind extra testing. But, I am curious about when they actually add value.
For example I could see using a wide variety of weapon sets to test out the performance of a new enemy to be valuable for balancing purposes. In that sort of case I could see 30+ tests being valuable.

@Night-Pryanik
Copy link
Contributor

I'm extensively testing (i.e. about 10 times) working/finished locations with some sort of randomization method in it, e.g. those with nested mapgen in them. There were lots of situations when 9 times out of 10 I debug-spawned such locations and it was everything ok with the them, and only on the last spawn I noticed some error. I didn't count frequency of such situations, of course, but it feels like "often".

@JonathanLochridge
Copy link
Contributor Author

Unless I misunderstand the term, I don't think this solution engages in nested mapgen. Although learning to work with that is something I am hoping to learn. It is good to know that I should test any attempts to work with it thoroughly. Although, it does seem logical that if you have a set of restrictions for how a set of tiles can dynamically connect and combine together that such a system might be prone to minor glitches that only occur when two particular tiles generate in a wrong manner.
The primary change of my varient only effects a single tile. And it's edge is unchanged so I don't see how a combination error could arise.
Although, on further reflection I realized that my smaller change to make the windows more symmetrical on the variant could result in less symmetry if it grabs a submap with the old window size in one place and the new one in another. However, even in such a case I don't think it would effect the functionality or look out of place in a way that might draw attention.
I think further testing shouldn't be needed here in this case. But I wouldn't be entirely opposed to more checking this weekend if I have the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions json-styled JSON lint passed, label assigned by github actions Scenarios New Scenarios, balancing, bugs with scenarios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants