Skip to content

Fix target nodes being added to the game XML file in reverse order for the `replace` and `append` modes

Snh20 requested to merge Snh20/payday2-superblt-lua:target-reversal-fix into master

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

Edited by Snh20

Merge request reports