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

Make generate_reflection save the fixture place automatically #181

Merged
merged 23 commits into from
Oct 9, 2021
Merged

Make generate_reflection save the fixture place automatically #181

merged 23 commits into from
Oct 9, 2021

Conversation

kennethloeffler
Copy link
Member

@kennethloeffler kennethloeffler commented May 28, 2021

enigo has some problems (at least on Windows)1, and to get this to work more-or-less reliably we need to do things with the WinAPI that no crate I'm aware of currently does... so we'll do it live!

This PR adds the struct StudioKeyboard that can send keystrokes to Roblox Studio, or indeed any process' top-level visible window. It does so by attaching Roblox Studio's input processing mechanism to the current foreground thread's via AttachThreadInput and sending keyboard events with SendInput. AutoHotKey also uses this technique.

The failure cases feel kind of bad as an end user who is probably using cmd or powershell, but it definitely makes it easier for us to use this tool and makes it possible to run it on a CI machine.

1For example, it uses a transmute_copy that invokes UB to create keyboard events. It also doesn't properly use SendInput - the entire point of this function is that it inserts multiple events serially into the input stream (which is why it takes an array), but enigo only passes one at a time!

@kennethloeffler
Copy link
Member Author

kennethloeffler commented May 28, 2021

I've just sprinkled in some cfgs to get this passing, but we'll need to add the Windows toolchain / target in the workflow if we want this to actually be checked.

It is UB for func to panic because it is called by the extern fn
callback.
@Kampfkarren
Copy link
Member

Kampfkarren commented May 29, 2021

Are you guys sure this isn't a better solution than setting auto-recovery interval to a minute and waiting until an auto-save is put into a folder? You should be able to do that with Lua (to set the settings) and safe Rust (to track the auto-saves).

@kennethloeffler
Copy link
Member Author

kennethloeffler commented May 29, 2021

This does work mechanically, but has a problem: Roblox Studio's autosaves are .rbxl files! generate_reflection currently uses rbx_xml to read the saved place file, using an option that completely bypasses the reflection database. rbx_binary does not have such an option. We'd need to think about this carefully and implement a similar mechanism in rbx_binary. I've generated the JSON database for inspection: as-is, some Int32 properties turn into BrickColors.

There are a couple other problems that are not addressed by this commit. First, the autosave directory will not necessarily be Documents/ROBLOX/AutoSaves (and we cannot change this from Lua - maybe it's in the registry somewhere?). Second, the existing property patches were created with .rbxlx files in mind and do not apply to .rbxl files.

@Kampfkarren
Copy link
Member

The autosave directory is a setting, I thought.

@kennethloeffler
Copy link
Member Author

kennethloeffler commented May 29, 2021

It's a QDir! Lua can't touch those.

@kennethloeffler kennethloeffler marked this pull request as draft May 30, 2021 00:54
It's likely that the user is off in another window editing a place
which will trigger an auto-save - very bad for us!
@kennethloeffler
Copy link
Member Author

The scope of this work has increased somewhat, so this is the last change I'll make here before splitting off tomorrow into rbx_binary and roblox-install to get the rest done. roblox-install will need to read and write Studio settings. I still need to look at what exactly needs to be done to start using rbx_binary here.

@LPGhatguy
Copy link
Contributor

This is starting to look like something I'd be down to ship!

Pending rbx_binary changes
@kennethloeffler
Copy link
Member Author

kennethloeffler commented Jun 5, 2021

I've removed all the earlier junk and replaced it with a compiled AutoHotKey script. The resulting executable contains the AutoHotKey interpreter in addition to the script and will run on a machine without an AutoHotKey installation. Notably, this obviates the need for both #183 and #182. This only solves the problem on Windows but I'll take it! If we want to do the same for MacOS in the future, we can look at using AppleScript.

I should really start running it automatically...
@kennethloeffler kennethloeffler marked this pull request as ready for review June 5, 2021 01:39
@kennethloeffler
Copy link
Member Author

Seems we've pretty well exhausted the possible solutions. We cannot rely on Roblox Studio's auto-recovery feature because it outputs .rbxl files - we need an .rbxlx to generate our database, otherwise it becomes extremely difficult to generate correct XML. Requiring an AutoHotKey installation feels bad. Just slapping in a compiled script feels bad. There are no existing crates that can ensure Roblox Studio receives our keystrokes.

The right solution seems to be AutoHotKey Except It's a Rust Library. I am starting work on a minimal crate that will provide functionality similar to 0269d7d so we can finally finish up this feature.

@kennethloeffler
Copy link
Member Author

kennethloeffler commented Jun 8, 2021

...and there we are. AutoHotkey does a very elaborate song and dance to handle the problem of modifiers being down during the send (and I have no doubt it's all necessary to properly do), so I chose to just ignore that for now.

Copy link
Contributor

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@LPGhatguy LPGhatguy merged commit 6a2fc02 into rojo-rbx:master Oct 9, 2021
@kennethloeffler kennethloeffler deleted the gen-reflection-autosave branch October 9, 2021 19:23
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.

3 participants