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

[FEATURE] add Blockly Dungeon #1066

Merged
merged 13 commits into from
Oct 24, 2023
Merged

[FEATURE] add Blockly Dungeon #1066

merged 13 commits into from
Oct 24, 2023

Conversation

cagix
Copy link
Member

@cagix cagix commented Sep 29, 2023

Das ist der Folge-PR zu #1061, der zur Integration des Blockly-Dungeon von @Kevin-Ratschinski in den Dungeon dient.

TODO:

  • Prüfung und ggf. Anpassung der Sourcen und Strukturen
  • Anpassen der Dokumentation (keine Verlinkung der Header, Einsatz von YAML, readme.md pro Ordner, ...)
  • Formatieren der Sourcen (Spotless war nicht ganz happy)

@cagix cagix requested review from AMatutat and a team as code owners September 29, 2023 08:18
@cagix cagix requested a review from a team September 29, 2023 08:18
@cagix cagix marked this pull request as draft September 29, 2023 08:22
game/src/core/Game.java Outdated Show resolved Hide resolved
@AMatutat
Copy link
Contributor

Prüfung und ggf. Anpassung der Sourcen und Strukturen

Habe die Änderungen im core in einen eigenen Starter verschoben (incl. build target blocky). Damit ist auf Codesicht die integration erstmal bedenkenlos machbar.
Von Node.js hab ich keine Ahnung, den Code kann ich nicht bewerten.
Den Server muss man aber noch manuell übers Terminal Starten (das ist so Webserver Magie)

Formatieren der Sourcen (Spotless war nicht ganz happy)

Hab ich mal laufen lassen, ist damit auch done.

@cagix
Copy link
Member Author

cagix commented Sep 29, 2023

@AMatutat Zwei Fragen:

  1. Wollen wir das neue Teilprojekt wirklich "blockly-dungeon" nennen oder einfach nur "blockly"? Letzteres fände ich besser (kürzer, prägnanter) - könnte uns aber evtl. Probleme machen, da es bereits ein Projekt Blockly gibt?
  2. Brauchen wir den Unterordner frontend oder kann der Inhalt eine Ebene höher verschoben werden?

Edit: Hmmm, es geht ja nur um Ordnernamen. Sollte also passen. ...

@AMatutat
Copy link
Contributor

AMatutat commented Sep 29, 2023

  1. blockly find ich auch gut.
  2. ich finde die Trennung nicht schlecht. In Frontend liegt der ganze Node.js stuff (da müssen wir ggf nochmal an unsere gitignore).
    Würde das schon gerne klar vom javateil trennen wollen

@cagix
Copy link
Member Author

cagix commented Sep 29, 2023

  1. blockly find ich auch gut.

dann "make it so" :)

  1. ich finde die Trennung nicht schlecht. In Frontend liegt der ganze Node.js stuff (da müssen wir ggf nochmal an unsere gitignore).
    Würde das schon gerne klar vom javateil trennen wollen

er hat ja ein eigenes gitignore in seinem projekt, vielleicht reicht das. aber ja, muss man prüfen.

bei den strukturen war meine motivation, hier die übliche maven-/gradle-struktur zu haben, die wir auch für die anderen java-projekte einsetzen, also unter dem projekt-ordner die ebene mit "src", "test", "assets", "doc", und in "src" dann die sprach-ordner (java, typescript) usw. im moment haben wir für blockly ein "src", und darunter direkt java-packages. und in "frontend" liegt die doku und eine webseite und noch ein "src" mit den sourcen/packages (nennt man das bei typescript so?) direkt drunter. fände ich schöner, wenn das einheitlich wäre. ggf. könnte man @Kevin-Ratschinski um eine entsprechende anpassung bitten?

edit: sehe grad, dass wir selbst nicht so richtig 100% maven/gradle sind. unterhalb von "src" kommen bei uns auch schon direkt die packages.

@AMatutat
Copy link
Contributor

edit: sehe grad, dass wir selbst nicht so richtig 100% maven/gradle sind. unterhalb von "src" kommen bei uns auch schon direkt die packages.

Ja, wir hatten das mal früher (teilweise dann noch mit dieser ewig langen org-struktur (de.dungeon.pm....schlag mich tot)

Ich verstehe jetzt aber wie du dir das vorstellst. Fände ich auch denkbar. In den anderen Projekten würde ich aber kein java Ordner einführen, da gibts nur java....

@AMatutat
Copy link
Contributor

@cagix ich hab jetzt die Dokumentation angepasst und den core unabhängig vom Blockly gemacht (da war vorher eine Abhängigkeit drinnen)

Was ich jetzt nicht gemacht habe ist, die Verzeichnisstruktur des Source-Code zu ändern. Also "frontend" entfernen und dafür src/java und src/typescript anzulegen.
Überwiegend weil ich keine Ahnung von den Implikationen habe und ich auch nicht wüsste ob nur frontend/src nach src/typescript gehört oder auch die ganzen json und html files (und wenn nicht, wo ich das anpassen muss).

Copy link
Member Author

@cagix cagix left a comment

Choose a reason for hiding this comment

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

@cagix ich hab jetzt die Dokumentation angepasst und den core unabhängig vom Blockly gemacht (da war vorher eine Abhängigkeit drinnen)

Was ich jetzt nicht gemacht habe ist, die Verzeichnisstruktur des Source-Code zu ändern. Also "frontend" entfernen und dafür src/java und src/typescript anzulegen. Überwiegend weil ich keine Ahnung von den Implikationen habe und ich auch nicht wüsste ob nur frontend/src nach src/typescript gehört oder auch die ganzen json und html files (und wenn nicht, wo ich das anpassen muss).

Hmmm, dann sollten wir das erstmal so lassen. Hast Du mal getestet, ob das noch läuft/startet?

Wir haben noch ein Problem: In ziemlich vielen Screenshots(?) ist das HSBI-Logo drin. Man kann sich über den künstlerischen Gehalt streiten, aber wir können das nicht einfach so hier ins Repo packen. Ich sehe zwei Möglichkeiten: (a) die Screenshots nochmal ohne das Logo aufnehmen oder (b) in Paint oder Preview o.ä. das Logo zu "übermalen" und die resultierenden Bilder einzuchecken.

Ansonsten aus meiner Sicht ok. Kann leider nicht approven, weil ich den PR erstellt habe 🙃

blockly/doc/img/Blockly_App.png Outdated Show resolved Hide resolved
blockly/doc/img/examples/bedingung.png Outdated Show resolved Hide resolved
blockly/doc/img/examples/charakter_bewegen.png Outdated Show resolved Hide resolved
blockly/doc/img/examples/komplexes_beispiel.png Outdated Show resolved Hide resolved
blockly/doc/img/examples/schleife.png Outdated Show resolved Hide resolved
blockly/doc/img/examples/skills.png Outdated Show resolved Hide resolved
blockly/frontend/README.md Outdated Show resolved Hide resolved
blockly/frontend/public/cat_logo.png Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
@AMatutat
Copy link
Contributor

(b) in Paint oder Preview o.ä. das Logo zu "übermalen" und die resultierenden Bilder einzuchecken.

Paint it is:

@Lena241 ich hab da was zu tun für dich ^^

@Lena241
Copy link
Contributor

Lena241 commented Oct 23, 2023

Ich habe alles fachmännisch mit weißer Farbe übermalt.

@AMatutat
Copy link
Contributor

Ich habe alles fachmännisch mit weißer Farbe übermalt.

vielleicht hat andre grade geforce pusht.... hast du das noch?

@Lena241
Copy link
Contributor

Lena241 commented Oct 23, 2023

Glücklicherweise ja.

@AMatutat AMatutat marked this pull request as ready for review October 23, 2023 13:32
AMatutat
AMatutat previously approved these changes Oct 23, 2023
Copy link
Contributor

@AMatutat AMatutat left a comment

Choose a reason for hiding this comment

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

lgtm
habs auch nochmal getestet gehabt

@AMatutat
Copy link
Contributor

hab nochmal gerebased, der kram könnte jetzt rein

Copy link
Member Author

@cagix cagix left a comment

Choose a reason for hiding this comment

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

die bilder scheinen jetzt ok zu sein, d.h. das könnte zu aus meiner sicht. (kein approval, weil ich technisch der pr-ersteller bin und nur noch kommentieren darf.)

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
@AMatutat AMatutat merged commit 54385f3 into master Oct 24, 2023
@AMatutat AMatutat deleted the blockly branch October 24, 2023 09:44
@cagix cagix added the blockly label Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants