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

Automapping: Always apply output sets with empty index #3909

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

bjorn
Copy link
Member

@bjorn bjorn commented Mar 15, 2024

Output sets with an empty index (output_foo, rather than output1_foo, for example) are now considered unconditional outputs and no longer participate in the random output index selection process.

Effectively, when a rule matches that has output sets with both an empty index and output sets with an index, first its unconditional output set will apply and then a randomly selected output set from its output sets with non-empty index.

For compatibility reasons, this behavior does not affect rule maps in "legacy" mode (when the user manually defined the rule regions).

@bjorn
Copy link
Member Author

bjorn commented Mar 15, 2024

One thing I'm wondering about, is whether it is alright for these rules to partially apply. Currently, when the NoOverlappingOutput option is used, the unconditional and random parts of the rule output can independently decide not to apply. Alternatively, I could first check the option for both and only apply both if neither of them overlaps previous applications.

@bjorn bjorn force-pushed the automapping-empty-output-index branch from 030007f to d7535d3 Compare March 15, 2024 16:49
@eishiya
Copy link
Contributor

eishiya commented Mar 15, 2024

The alternative option sounds more intuitive to me.

bjorn added 2 commits March 18, 2024 15:29
Output sets with an empty index (output_foo, rather than output1_foo,
for example) are now considered unconditional outputs and no longer
participate in the random output index selection process.

Effectively, when a rule matches that has output sets with both an empty
index and output sets with an index, first its unconditional output set
will apply and then a randomly selected output set from its output sets
with non-empty index.

For compatibility reasons, this behavior does not affect rule maps in
"legacy" mode (when the user manually defined the rule regions).
Now that rules can have an unconditional output along with a random
output, make sure that they are either both applied or neither,
concerning making sure there is no overlap with previous applications.
@bjorn bjorn force-pushed the automapping-empty-output-index branch from d7535d3 to 6ada9a7 Compare March 18, 2024 15:22
@bjorn bjorn merged commit 88e4525 into mapeditor:master Mar 18, 2024
13 of 14 checks passed
@bjorn bjorn deleted the automapping-empty-output-index branch March 18, 2024 16:34
@bjorn
Copy link
Member Author

bjorn commented Mar 18, 2024

The alternative option sounds more intuitive to me.

Yeah, I've changed the behavior. And more importantly, fixed a breakage where rules without indexes would no longer apply at all, heh. Also I hope the docs updates are clear.

@eishiya
Copy link
Contributor

eishiya commented Mar 18, 2024

Perhaps the docs for updating old rules needing explicit indices could link to the index-adding script I submitted to tiled-extensions?

I think the documentation is fairly clear, Since these changes reflect the way people tended to assume Automapping worked, I worry spelling them out explicitly might actually confuse people 🤣 I think that eventually, perhaps after a couple of minor versions, this information should be incorporated into the main description for output indices, rather than being described as a change, or at least reworded to de-emphasise the fact that it's a recent change.

@bjorn
Copy link
Member Author

bjorn commented Mar 18, 2024

Perhaps the docs for updating old rules needing explicit indices could link to the index-adding script I submitted to tiled-extensions?

Right, of course! (done in #3912)

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.

2 participants