godotengine / godot-vscode-plugin

Godot development tools for VSCode
MIT License
1.57k stars 168 forks source link

Debugger Fixes for Godot 4.0 #456

Closed QuinsZouls closed 1 year ago

QuinsZouls commented 1 year ago

I been working on godot 4 support, also thanks to @DaelonSuzuka, @RedMser, @ZachIsAGardner for their work (I included some of their changes), here's my changes and implementations:

Debugger

DaelonSuzuka commented 1 year ago

Hi @QuinsZouls,

Thank you for you interest in improving this extension! I appreciate that you took the time to put this together and submit it.

Unfortunately, there are a number of problems with this PR that mean it can't be accepted.

Please do not combine multiple PRs together!

There are a lot of reasons not to do this, but one of the biggest is that you end up with a giant PR that touches way too many things, and that makes it much harder for other people to review it effectively.

It's important to understand all of the changes that are being made to a project, and that's more difficult to do if you're combining a bunch of other patches. As an example, PR #452 is highly experimental and DOES NOT WORK. It's marked as a draft because it's currently in-progress and very, very broken. The fact that it's included makes me seriously question if anything in this PR was tested.

The changes to tsconfig and the package updates are interesting, but I can't properly evaluate that stuff with how much other content is here. If you resubmitted those changes in a new PR by themselves, it's likely we'd be able to accept them.

QuinsZouls commented 1 year ago

Well, I'd to make single PR for each patch, thanks for you feedback, also I show you how my changes work (I didn't just merge all code, during the merge commit I made some fixes):

https://user-images.githubusercontent.com/40646096/222931901-a3ad6b59-4052-411c-a7fa-b3b668e2a1bc.mp4

QuinsZouls commented 1 year ago

I split all icons changes to a new PR This PR now focuses on debugger compatibility for godot 4.

QuinsZouls commented 1 year ago

BTW My changes made it work your PR #456

atirut-w commented 1 year ago

Well that's sad