search

Auroriax / PuzzleScriptPlus

Puzzlescript Plus: Open Source HTML5 Puzzle Game Engine (with lots of extra functionality)
https://auroriax.github.io/PuzzleScript/
34 stars 4 forks source link

Get latest changes from Increpare #23

Open Auroriax opened 2 years ago

Auroriax commented 2 years ago

https://groups.google.com/g/puzzlescript/c/pLY3PXjA9Gw

No big major changes but mostly just Quality of Life stuff. There's some fun stuff like the double ellipses though! Probably still a lot of work to merge it all, sadly...

Auroriax commented 1 year ago

Will probably start to slowly start chipping away on this. At moment of writing, the latest increpare commit merged into PS+ is f3655f2 - Merge branch 'master' of https://github.com/increpare/PuzzleScript.

Also clean Git history while I'm at it, because that thing is getting very very messy with all these incremental merges.

Auroriax commented 1 year ago

Probably a good idea to target a08d7b4 - new unit test, censored another to make it SFW first, since the date of that commit corresponds to the date the changelog was posted

david-pfx commented 1 year ago

I took a look at imerge. The idea is great, but it looks a bit clunky now compared to some of the other tools, including VS Code. Have you tried anything others?

Auroriax commented 1 year ago

I tried to find something more efficient from imerge, but I got the feeling that git is not really intended to resolve merges that are as divergent as this, so I assumed there was no easier way. (Might be that a new tool for this has popped up in the last year or two, though!)

As for merges, generally I want PS+ to be a subset of PS where-ever possible, which means that everything from PS should be kept as far as is reasonably possible.

I'd recommend to use imerge and then merge not the entire branch at once, but pick a commit like around ten commits upstream (ideally one you know that's stable, such as a bugfix or vanilla PS release), then test it in the browser once the imerge is completed. This should help you safely merge the thing while still being able to test it regularly.

Please work towards merging the vanilla PS releases and make PRs for each of those once you reach them. Or if you get stumped somewhere, submit your progress up to that point as a PR. This will also help me validate if the merge was performed correctly. See #23 for a good first release to merge towards.

There are instructions in the readme for how to contribute to PS+ if you already have a fork of vanilla PS. 😄

david-pfx commented 1 year ago

Surely PS+ should be a superset: all of PS plus extras? Did you mean downstream? Latest increpare merge is 28/9/21; 8 checkins downstream from there is all of Jan 21, then there's like 20+ checkins a month ending up on 17/3, then a gap and only a few more to the present time. There are heaps of changes to games_dat.js. You want to just ignore those? Really? Demos: there are no 'PS+ bits' that I can find, just lots of incoming changes and random white space differences. Trivial merge. Perhaps the PR notes should be in 'develop', I didn't notice them in readme. Will do.

david-pfx commented 1 year ago

Can someone please tell me what goes with strict mode? Compiler.js is strict mode and parser.js is (mostly) not. Why?

david-pfx commented 1 year ago

OK, I have a working merge up to 23/2/22. Apart from a couple of minor issues there are two big problems: the gallery won't merge and the test cases won't merge. It's likely possible to force fit them but the exact same problem will arise on the next merge. That looks like an ongoing wasted of time, to no good purpose. My proposal is to use the PS gallery and PS test cases and get it all working. Then add back the PS+ gallery and PS+ specific test cases but keep them separate, in their own files/folders. There will be pain around case-insensitive, but I can fix that too. The end result will function as now, maintain a near-perfect level of superset compatibility with PS, but merge more easily in future.

david-pfx commented 1 year ago

It now seems that the legend and sounds sections in parser.js will not merge. Did someone rewrite them? The PS versions look like they've had a lot of work.

Is there any reason not to just use the PS version? Does PS+ need anything special in those sections?

Auroriax commented 1 year ago

Before you continue, could you submit what you have as a (draft) PR? I want to take a look at the issues and questions you're having. In hindsight I kinda feel like I threw you into the deep end, it would probably be better if I took over the merge from this point as you seem to be running into a lot of difficult issues.

What kind of approach to merging are you using? imerge or something else?

I don't know the answers your other questions, sadly. Sorry!

david-pfx commented 1 year ago

I'm happy playing in the deep end a bit longer before you send in the lifeguards. It's where I mostly hang out.😁 I'm doing a partial merge up to 23 Feb. From what I read of imerge it somewhat automates the easy bits and won't help with the hard bits. In any case I need to improve my skills on Git merges. FYI my main tools are VS Code, TortoiseGit and BeyondCompare. As I said, I don't think it's feasible (or desirable) to merge the gallery or test cases. The PS+ ones need to be kept somewhat separate and I would like to discuss how to do that. It seems Stephen made bigger and deeper changes to the parser than we realised, and that has a big effect on tests that target error messages. I can do the parser merge and get all the PS tests passing, but I badly need PS+ specific tests for each of the features listed in the readme (not the ones created by contamination with case insensitive).

david-pfx commented 1 year ago

I have a working merge of Increpare 4d9d5 23-feb-22, which passes all PS tests. I do not currently have any PS+ tests. The code still has some place markers, to be resolved during the next merge(s).

The code is in https://github.com/david-pfx/PuzzleScriptNext on branch master. I have not been able to create a PR, using the instructions in the README. Please advise.

Auroriax commented 1 year ago

Your repo doesn't seem to be a fork of anything at the moment (as it's not displaying the forked from increpare/PuzzleScript text in the top), so likely you don't get an option to make a PR. If you do that you should be able to make PRs to there and here. (I'm not sure you can do this if the repository already exists?) Then you should be able to pull a branch from this fork locally, work there, and make a PR here. I can try to give a more detailed explanation if you need it.

I believe there's a hacky way to get around it and create an organization, and fork from there. Which might be easier in your situation.

I'll pick up the extra tests as part of #31, see my last comment there for more info

david-pfx commented 1 year ago

My local repo is a clone of PS+, with an imported branch for PS, from which I merge onto a tomerge branch and then onto master. I now have 3 working merges, covering most of the older PS stuff. But the problem is how to turn that into a PR.

I can't do a fork of PS+ on GitHub because I already have a fork of PS (which I plan to keep). I thought the instructions in the README were supposed to deal with this but (a) I don't find them very clear and (b) everything I tried doesn't work. I can't push directly to PS+, and although GitHub seems to support doing PRs from one user to another I can't get it to work. Maybe this is something GitHub cannot do?

It should be possible to create a PR locally using TortoiseGit, but I'm having trouble with that too. It just hangs.

Any suggestions welcome.

david-pfx commented 1 year ago

I have now merged changes up to and including 7-mar-22. I have pushed the merge to my repo commit a62511640b9a87dc91ba0509e1b2688c08dbe583. It passes all PS tests. I expect it to fail some PS+ tests, particularly those related to case sensitive, but I have none. When they are available, I'll fix those too.

I don't think it's possible for me to create a PR on GitHub. In principle I should be able to create one locally but so far it doesn't work. I can fix the code, but mastery of Git is another thing entirely.😁

david-pfx commented 1 year ago

Progress report: all Increpare changes merged. All new PS+ tests merged. All the tests pass and everything is working except:

  1. Case insensitive is broken by the new changes, so Kye fails. Badly, because the code is really fragile. Working on it.
  2. The Node compile is still outstanding.
david-pfx commented 1 year ago

Actually that's not quite right: Sorting of Sorts fails too. I missed it because it was dying quietly (problem with the test harness). Can you please provide source code for these two games, that you turned into test cases? They're hard to debug without.

Auroriax commented 1 year ago

Right, so regarding the fork, you have two options.

Note that you can't turn a repo into a fork after creating it (Source: https://stackoverflow.com/a/56086243 ). If you still want this, you might be able to migrate the git history or something. If you want to create a PR then both repos need to be connected to the same project (e.g. the lack of the "forked from increpare/PuzzleScript" text on the project you linked is likely what is preventing you from creating a PR).

New branch in existing fork

In your existing repository for PuzzleScript, add PS+ as an origin and then pull a PS+ branch.

  1. On your local copy of vanilla PS, add PS+ as an origin. (In the screenshot I'm using your repo as example. Screenshot was made with Fork, but other GUIs should be similar) afbeelding
  2. Fetch, then pull a PS+ branch.
  3. Start working on the branch (or create a new branch first). Once you make a commit and push the branch, you should have the option to create a PR on GitHub.

Create an organization and fork there

If you don't want to add a branch in your existing repository, you can also create an organization and set up a fork there. https://docs.github.com/en/organizations/collaborating-with-groups-in-organizations/about-organizations. Then you should be able to press the Fork button and create the fork in the organization.

You should be able to find hack links for all games in the test cases via either the examples, the PS+ gallery, and my itch.io pages (see e.g. https://itch.io/c/1609485/made-with-puzzlescript-plus)

david-pfx commented 1 year ago

Thank you, that worked. I pulled your develop branch, merged my changes onto it, pushed it to GitHub and created a PR on that (which I hope meets your needs). Please note:

Auroriax commented 1 year ago

I don't see any PRs coming in yet. Have you made sure to target Auroriax:develop?

david-pfx commented 1 year ago

I guess that would explain why I haven't heard from you.😀 This is the PR: https://github.com/david-pfx/PuzzleScriptNext/pull/1. If I've missed a step in getting it to you, please tell me what that is.

Auroriax commented 1 year ago

I'm still not seeing it come in here. Please consider reading up on PR documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request, "Further Reading" links on that page, other PRs on this repo, etc.

david-pfx commented 1 year ago

This is getting frustrating. I've already done everything on that page (that I am allowed to do). I have created the PR. I have posted the link in this issue. I do not have the rights to link the PR to the issue, if that's what's needed. So please explain: what does it mean to "come in here"? Where is "here"? Which specific part of the voluminous GitHub help tells me how to find the missing step between creating the PR and putting it in the right place for you to see it?

david-pfx commented 1 year ago

Sorry it's taken so long, but I finally figured out the rules. With a little help from my son.😁

pr-plus-#23 submitted.

david-pfx commented 1 year ago

PR submitted but not yet acted on. Not much more I can do.

Meanwhile see https://github.com/david-pfx/PuzzleScriptNext and https://david-pfx.github.io/PuzzleScriptNext/src/editor.html for a working version with all changes merge.