Fix target nodes being added to the game XML file in reverse order for the `replace` and `append` modes
Hello, it seems that if multiple elements are specified in the target tag (such as when
replacing a single element with multiple new elements), they will be added in reverse
order. This affects the replace
and append
modes, but not the attach
and
attribute
modes. Therefore, to avoid surprising the
user with this behavior, I've changed the code in question to iterate in reverse so the
nodes will be added in the expected order.
However, due to how long this behavior has been present (if I'm not mistaken, since commit bc0f8db1 was made 7 months ago), I'm also a bit concerned with how this change may impact users out in the wild by potentially breaking their tweaker files, since they may have worked around it by inverting their own tweaks.
If potential breakage does prove to be an issue, perhaps a new reversed
attribute could be
added to the <target>
tag to preserve original behavior, with the default set to true
?
Demonstration
Given the following excerpt from a pristine copy of core/shaders/post_processor.post_processor
:
(irrelevant lines have been omitted for brevity)
<?xml version="1.0" encoding="utf-8" ?>
<post_processor>
...
<effect name="bloom_combine_empty">
<defined platform="PS3 X360">
<texture_filter src="bb" dst="albedo" filter="identity"/>
</defined>
</effect>
...
</post_processor>
And the following example tweak (intending to add three new child nodes to <effect/>
, all on the same level as <defined />
):
<?xml version="1.0"?>
<tweak name="core/shaders/post_processor" extension="post_processor">
<search>
<?xml version="1.0" encoding="utf-8" ?>
<post_processor />
<effect name="bloom_combine_empty" />
<defined platform="PS3 X360" />
</search>
<target mode="append">
<texture_filter src="low_target_2" dst="low_target_1_b" filter="identity"/>
<texture_filter src="low_target_3" dst="low_target_1_b" filter="identity"/>
<texture_filter src="low_target_4" dst="low_target_1_b" filter="identity"/>
</target>
</tweak>
With the current code, this results in:
<?xml version="1.0" encoding="utf-8" ?>
<post_processor>
...
<effect name="bloom_combine_empty">
<defined platform="PS3 X360">
<texture_filter src="bb" dst="albedo" filter="identity" />
</defined>
<texture_filter src="low_target_4" dst="low_target_1_b" filter="identity" />
<texture_filter src="low_target_3" dst="low_target_1_b" filter="identity" />
<texture_filter src="low_target_2" dst="low_target_1_b" filter="identity" />
</effect>
...
</post_processor>
With the changed code, this instead results in:
<?xml version="1.0" encoding="utf-8" ?>
<post_processor>
...
<effect name="bloom_combine_empty">
<defined platform="PS3 X360">
<texture_filter src="bb" dst="albedo" filter="identity" />
</defined>
<texture_filter src="low_target_2" dst="low_target_1_b" filter="identity" />
<texture_filter src="low_target_3" dst="low_target_1_b" filter="identity" />
<texture_filter src="low_target_4" dst="low_target_1_b" filter="identity" />
</effect>
...
</post_processor>
In some cases, the order of the new nodes has no effect on the output, but in others (particularly the rendering-related XML files) it can have a significant impact on the result, which can cause user confusion as the resulting file may not actually match their expectations.
While they could modify base.wren
to dump the changed file
to log output, it seems unlikely that that would be the first approach they consider,
much like how developers don't normally begin to investigate bugs by blaming the compiler
or CPU. :)
Thank you for your time.
Edited to clarify that attach
mode is unaffected by the issue