ubc-minetest-classroom / minetest_classroom

Repo for Mintest Classroom game
GNU General Public License v3.0
10 stars 6 forks source link

Unified GUI updates, part 3 #231

Closed Lowie375 closed 1 year ago

Lowie375 commented 1 year ago

This PR includes a bunch of fixes for identified issues with the current GUI, as well as various new GUI-related features and feature implementations.

Notable inclusions:

Lowie375 commented 1 year ago

Some features are still incomplete: these will be added in a later PR (which should hopefully come soon!)

pauldpickell commented 1 year ago

The new tab system for classrooms public/private on the student notebook creates an interesting wormhole where I can move a classroom to hidden as teacher, teleport there, and then return to student notebook to find I belong to no classroom. This is intended behaviour and does not seem to cause any issue, though it might have unintended consequences/confusion.

pauldpickell commented 1 year ago

When I edit the spawn classroom and change it to private, then set another classroom as spawn, I still cannot hide the original spawn classroom. But I can hide any other classroom that I designate as spawn.

Lowie375 commented 1 year ago

When I edit the spawn classroom and change it to private, then set another classroom as spawn, I still cannot hide the original spawn classroom. But I can hide any other classroom that I designate as spawn.

I suspect this is because setting a classroom's type to "spawn" doesn't actually change which classroom is recognized as the spawn in mc_worldmanager (which manages classroom teleportation). Will work on making the requested changes over the next few days

Lowie375 commented 1 year ago

The new tab system for classrooms public/private on the student notebook creates an interesting wormhole where I can move a classroom to hidden as teacher, teleport there, and then return to student notebook to find I belong to no classroom.

A couple options worth considering:

pauldpickell commented 1 year ago

The new tab system for classrooms public/private on the student notebook creates an interesting wormhole where I can move a classroom to hidden as teacher, teleport there, and then return to student notebook to find I belong to no classroom.

A couple options worth considering:

* Add a "hidden" tab to the student notebook for students currently in a hidden realm (with only their current realm shown), then hide that tab once those student leave the realm

* When a realm is hidden, send a message to everyone currently in the realm: everyone gets told that the realm has been hidden, students also get told that they won't be able to re-enter the realm once they have left it

I think another option is to just check that the realm is empty of all players, then allow it to be hidden. That would solve any issue of a player being in a hidden realm. Teachers can always move players out of realms if needed. Alternatively, we could just force teleport players out to the spawn realm when a realm is marked as hidden, in the same way that happens when realm deletion is initiated.

pauldpickell commented 1 year ago

I just teleported to a realm on the hidden tab, which moved my player to that realm, but when I closed the teacher controller GUI, it instantly teleported me back to spawn. Now when I open the classroom tab on the teacher controller, I can flip between spawn and hidden realms and it shows (1 player) for both. Then, I tried teleporting back to the realm again, and it returned me to spawn every time. So I tried another realm that I had hidden, and it seems I was able to successfully move around without being teleported back to spawn. However, the hidden tab formspec list shows (1 player) now for both realms.

Then, I decided to create a new empty realm and moved it to hidden for testing. Once I did that, the realm I was trying to teleport to completely disappeared from the list. After some thought, I realized that this realm I was trying to teleport to had probably been deleted when I was testing the PR, but it was not removed from the list (difficult to tell because all realm names are the same). So realms deleted may not be removed from the hidden list.

Lowie375 commented 1 year ago
  • [ ] Disallow changing the spawn classroom to private; I was not able to test this in singleplayer mode, but this could prevent non-teacher players from joining/spawning correctly

Instead of doing this, I've added some additional checks on join that make sure players have access the the realm they were in when they left the game. If they can't rejoin their previous realm (due to realm category changes, for instance), they will be moved to the spawn realm (which is joinable by all players, and will always exist, since a new one is generated if the existing spawn realm's category is changed)

Lowie375 commented 1 year ago

I think another option is to just check that the realm is empty of all players, then allow it to be hidden. That would solve any issue of a player being in a hidden realm. Teachers can always move players out of realms if needed. Alternatively, we could just force teleport players out to the spawn realm when a realm is marked as hidden, in the same way that happens when realm deletion is initiated.

I'm going to do something a bit between these two approaches: if someone tries to hide a realm with players inside of it, they will be shown a popup asking if they want to hide the realm and kick out any players currently in it or cancel the hide action.

pauldpickell commented 1 year ago

Some additional testing with hosted server reveals that the textlist of connected players in the players tab of the teacher controller is not reloaded when you open/close the tool. The only reload I have observed is when you select different tabs (students/teachers/classrooms). The effect is that stale player names remain in the textlist context.

To reproduce:

pauldpickell commented 1 year ago

Another unexpected behavior: join a hosted server as a student and you are given tutorial notebook and student notebook tools. Leave the game and then rejoin and both tools are taken from inventory without applying any priv changes from teacher controller.

Edit: appears that student priv is somehow revoked when rejoining the server. Edit2: granting the player the student priv from the teacher controller works as expected.

pauldpickell commented 1 year ago

Instead of doing this, I've added some additional checks on join that make sure players have access the the realm they were in when they left the game. If they can't rejoin their previous realm (due to realm category changes, for instance), they will be moved to the spawn realm (which is joinable by all players, and will always exist, since a new one is generated if the existing spawn realm's category is changed)

I just tested this by creating a realm, teleporting another player there, leaving the game with that player, then hiding the realm, and rejoining, and the player is able to rejoin the realm that they left the game in, which is now hidden. Expected behavior is that the player's realm is checked if it can be joined and if not then teleported to the spawn realm.

I also tested by changing the realm type to private, but this resulted in damage to the player on rejoining, dying, and then spawning into the spawn realm. Again, the player should just automatically be teleported to spawn instead of dying.

pauldpickell commented 1 year ago

Freeze/unfreeze works great except when the player is frozen in a classroom that is hidden. When this happens, the player is gets the usual chat message that the classroom has been hidden and you were brought back to the spawn. But the player is not automatically teleported to the spawn realm, instead dies, and then when they do respawn in the spawn realm, they are not frozen. This causes the unexpected behavior that the player can seem to move around, but cannot teleport to any classrooms and from the teacher controller the player still shows as having the frozen state.

Also, we might think about adding a chat message or something that indicates to the player that they are frozen.

pauldpickell commented 1 year ago

All terrain generators behave as expected except DNR. DNR was meant to be map regen method and so it relies on some nodes already being defined in the realm. Apparently, if the realm is generated with DNR, the behavior is that the entire realm is filled with dirt and then the player spawns outside of the bounds of the realm, which results in a loop of damage and respawning outside the realm. For this reason, DNR should be removed from the terrain generator list.

Regen methods should be moved to the edit classroom popup. In other words, preserve the current realm settings and simply change the nodes using the terrain generators and decorators. The seed, sea level, generator, and decorator are all stored in realm.MetaStorage.

Lowie375 commented 1 year ago
  • [ ] 2. Players join hosted server receive student priv, then lose it when they leave/rejoin

Suspect this may be happening because student isn't being granted universally, that should be a simple fix

  • [ ] 3. Improper handling of cases where a player leaves, has their realm hidden or made private, and they rejoin with unexpected behavior
  • [ ] 4. Freeze/unfreeze fails when a player is frozen in a realm that is hidden.

This should be able to be fixed with some more checks, since the current checks are only using realm:getCategory().joinable() - will change behaviour for hidden realms and frozen players. Will also make sure players remain frozen on respawn (and that any other similar effects get applied on respawn)

[ ] 5. DNR and other regen methods need to be moved to classroom edit popup

Will remove DNR for now. It'll get added back into the edit popup once the infrastructure for regenerating realm terrain via the GUI is in place