atl transform in has block is misplaced

[madeddy]

Encountered a rare case of a atl transform in a has block (has fixed) and is then printed 26 lines to late. Looks for me as if its even printed afters its children and over-indented. AST says the at transform should land on line 162 but does on 188.

I try'd to analyze this by putting some debug prints in sl2_decompiler.py and managed to follow it:

• When the renpy.sl2.slast.SLDisplayable comes into print_displayable() it finds in sl2decompiler.py#L217 the ATL-transform and stores it in the variable
• Then it runs over #Line 236 into print_keywords_and_children()
• There it goes in #Line 389, instead of going to the "at transform" write at the end of the file
• From there it runs into print_node, from where it jumps back into "print_displayable()". Here he finds this time no ATL-transform in #Line 217.
• Why the transform then anyway writen is and after more other code beats me.

Added example files:

• .rpyc is original
• .rpy and astdump are with the currently updated py2 dev branch (as of 12.Feb. MEZ/CET)
• Src test game is Our Bright Days v0.1.3; Can be freely downloaded on their Patreon/Discord

OurBrightDays-0.1.3_quick_menu_scripts.zip 34,6 KiB

[CensoredUsername]

That indentation is definitely wrong. Currently at transform always gets dumped at the end of a displayable, that's just a todo in the code right now. But what's going wrong is that it seems to think that no block has been started yet by the time it needs to print at transform which triggers it to start a block there (see double colon on the line before the at transform as well.

Christ screen lang parsing is so terrible. There's so many corner cases that you just really don't want to think about. Can has have properties on the same line? Man this code is a pain. And I probably wrote it myself.

Also if you want to refer to which commit you used, you can just use the hash in here (like c9905445) as github auto links it. Makes it easier than doing timezone math.

[madeddy]

Makes it easier than doing timezone math.

This sounds like a advantage. 😁 Didn't think to use it like this. Thanks for the tip.

see double colon on the line before the at transform

Well double is new to me. I got one on the at ...line before and this is the first thing i noticed as it caused the error running the game. That the at transform wrong placed was noticed someone else. See:

                        at tr_player_playing_emoji(delay=.3, id=track['name']):
                    at transform:

Currently at transform always gets dumped at the end of a displayable

I thought about putting the at transform part at sl2.py end in a method so we can link it from where its needed, but i don't know if the ast state after this correct is or other problems arise with this strange atl callback fnc.

And I probably wrote it myself.

* Laughs * Hm. Some. In _printdisplayable is maybe half the code from you and in _print_keywords_andchildren its maybe a quarter now. Most is JackMcBarns work or based on yours and this mix is IMO part of the problem in this two methods.

If i look back in 2016 there is not even half the code in this part. 😲 What to do with this is a question for the py3 branch i think. Now we need a just a fix i'd say. When the py2 branch runs error free again it is done for good i recon and beyond we see just a few bugfixes for seldom code cases i think.

[CensoredUsername]

Just writing this down for my own sanity, the issue seems to be that the logic for the has statement isn't quite correct. In a normal displayable taking a block, The structure is as follows:

displayable_name positional_arg* (keyword_arg value)*:
    (("as" name) # only for screen:
    | ("tag" name) # only for screen:
    | (keyword_arg value)
    | "at transform:" transform
    | child)*

With no particular order requirement for items in the block.

However after a has statement, rules change a bit.

has can have a block, and any keyword properties or at transform statement needs to be in that block. Any children the has statement has then should not be in that block, but after the has statement at the same indentation level. Right now most of the same logic for that is reused, This, as you can imagine, causes some issues.

I tried to see if the ren'py parser for this provides more clarity than the docs do, but honeslty, tracing that code is even worse.

[madeddy]

Just writing this down for my own sanity

Quit alright. It's useful to do something like this. Could you add the pseudo* for our special case? Could help to get the picture.

However after a has statement, rules change a bit. Maybe we should at this point, because of this change, just fork out of the current code and write the logic for this separate. With possible reuse of current code there. Just to stay sane... and its perhaps faster done for now.

Right now most of the same logic for that is reused

Thats why i would like to break the code in _printdisplayable / _print_keywords_andchildren apart and put the pieces possible in methods. If doable. But again, a py3 problem for later.

tried to see if the ren'py parser for this provides more clarity

Well good luck with this. I tried for hours on end to get there something. The Ren'Py devs have their own style...and its the 20th year with all this code.

[CensoredUsername]

Decided to dig through the parser fully again, I guess the docs are just disagreeing with the actual implementation.

The has block is parsed the same as a normal displayable. It just first parses the block of the has statement (if any) and then parses the remaining block that the has statement was in. Keyword properties (also at transform, as it is handled like a keyword, except when it isn't), and children can be either in the block of the has statement or afterwards.

So putting at transform at the end for now should be valid, and technically has should never require a block. Now just to reverse engineer the line number logic for that again.

[madeddy]

So you got it. Respect. What you describe sounds like they hacked on multiple accounts their own engine. The has_block is a displayable and the atl-transform is sometimes a kw or not. Nice. The linenumbers are a headache sometimes.

I removed also the pull/branch for the at-transform nonsense. The remaining one commit for init.py was this:

                # HACK: This fixes the two statements on a line Error e.g.
                # `at tranform:parallel:` or `at tranform:on show:`
                # This is however probably a bad solution. The real reason
                # is the permanent activated `skip_indent_until_write=True)`
                # in line 134.
                self.skip_indent_until_write = False

Still in here: 2a26889 I am curious if this more substance has. Edit: Oh, i forgot. This was this issue: #115

[CensoredUsername]

I rewrote the SL2 handling significantly in 577c32e. The old one was a mess, and tbh, i couldn't for the life of me figure out how it was even meant to work anymore. Small cosmetic changes in the output (we never create a block after a has statement) and some relocations on tag/as keywords, as well as at transform statements now actually appearing at the right line. I think this rewrite also fixed statements appearing on the same line as at transform. This was likely caused by the hackish way atl is implemented in sl2, where call back into the main decompiler and just overwrite some state. Most notably, we did not overwrite skip_indent_until_write.

I'd like to pull atl decompilation out of the main decompiler into a separate one that both can call into to remove this hack to begin with. But for now, this works.

[madeddy]

Whoa! That was way more work as i thought. Hope you had fun. 😬

...i couldn't for the life of me figure out how it was even meant to work anymore.

TBH, it's a relief to read this. I starred for hours on this code, wrote changes which didn't "really" work in the end and tried to make sense of it. Good it's done.

Alltogether: If i count correctly there where at least four(!) bugs (we noticed so far) related to this. You cut most already out of the issues as i noticed.

I'd like to pull atl decompilation out of the main decompiler into a separate one...

Sounds logical to me. Also, please keep in mind there is more ATL changes in Ren'Py coming if I've seen this right. Related to parameter.py -> https://github.com/renpy/renpy/pull/5237

General q.: For which Ren'Py versions do we need testcases and which, if possible, and how many files?

[CensoredUsername]

General q.: For which Ren'Py versions do we need testcases and which, if possible, and how many files?

I try to just collect all i can for my personal testcases, I have like 1400? rpyc files from various versions. Sharing that is ofc an issue with copyright so it isn't public. But personally my approach to this project has always been to just fix stuff when people hand in files that don't work. If backwards compatibility gets broken at one point and someone cares, they can file an issue for that as well.

Anyway, closing this because it's fixed.