Add interactive "Door" objects. #12

helioscope · 2024-10-23T03:33:40Z

Adds "Door" object, extending FinishFlag as another way to transition between levels (or between different places in the same level). Requires a button-press to activate. There's quite a bit of refactoring in here to avoid redundant code (with NPC interactivity, with FinishFlag level-exit behavior, with the FinishFlag tooltip, etc). Currently things are still configured so that doors and flags can both target each other. (This configuration is just-barely leading in the poll right now, and where I'm leaning too if we revise the level-entrance picker to first choose the destination level, then pick the destination object.)

Some things to take a look at:

PlayerInteractionHandler.js -- handles drawing of arrows, targeting one interaction at a time, whether or not a player can and will interact, whether or not a player is allowed to jump (when there’s an interaction)
PlayMode.playerExitLevel(customExit) — a static method to trigger exiting a level. In the future, maybe this could be used for dialog and events. (I would also like to specify the transition here, so that doors can have a different transition than finish flags, and so specific transitions could be used in dialogs and events. I haven't gotten around to this yet.)
Hidden option for changeableAttributes that are listed in SpritePixelArray.js -- set this to true to skip over them in the ObjectToolTip (instead of type-checking for FinishFlag and Door). (We could instead create a new formElement type (e.g. “custom”) to achieve the same effect and be clearer about what’s happening, but I’m not sure how much it matters — being to hide attributes in general this way also seems potentially handy)
htmlRenderers/LevelEntrancePicker.js -- the level-entrance picker used by the ObjectsTooltipElementsRenderer for both FinishFlags and Doors
Door class
FinishFlag revisions
NPC revisions (blinking arrow indicator and activation moved to PlayerInteractionHandler)

…hFlag and StartFlag-related code to be shared more cleanly with Door.

…ons and UI/arrow-markers (refactoring Door and NPC); add playerExitLevel method to handle level exits (refactoring Door and FinishFlag)

…button as door trigger

… merge conflicts)

l0bster2 · 2024-10-28T18:10:21Z

I understand it's a WIP, but i just write some things down i realized while testing, in case you didn't know them:

the default NPC indicator became arrow down, it should stay arrow up
exported game is not working (DOOR_TRIGGERS is not defined)
Would be nice if the text was Activation-trigger, not in camel case. Also i would add the class "subSection" to this div, to have a margin and a line separator

l0bster2 · 2024-10-28T18:10:16Z

PlayLoop/JumpHandler.js

+        if (Controller.jump) {
+            if (!PlayerInteractionHandler.willAllowJump() || PauseHandler.justClosedPauseScreen) {
+                // no jumping, sorry
+            }
+            else if (player.swimming) {


why not just keep the condition as it was?

Suggested change

if (Controller.jump) {

if (!PlayerInteractionHandler.willAllowJump() || PauseHandler.justClosedPauseScreen) {

// no jumping, sorry

}

else if (player.swimming) {

if (Controller.jump && !PlayerInteractionHandler.willAllowJump() || PauseHandler.justClosedPauseScreen) {

if (player.swimming) {

If that's not possible, i would rethink how to make this condition. not a fan of empty IFs :P

This is 99% just personal style choice for readability. Shorter conditional lines and nesting less deeply tends to make things more readable, in my experience. There are other approaches than a comment-only if, though.

hm, but in that case it actually also changes the logic which condition fires. with my condition, the code wouldn't go inside the outer if. with your condition it would, as there is no return or something. and then it would trigger the "else" inside.
i don't know the consequences right away, but i would keep my condition tbh.

Oh, right! Actually one of the consequences of this (making the outer condition just be Controller.jump by itself) is that it prevents a bug where you can accidentally jump after passing interactable objects with the jump button already held down. It happens in the main branch right now and isn't too much of an issue there, but it can get weird in some edge cases with doors.

l0bster2 · 2024-10-28T18:17:03Z

PlayLoop/PlayerInteractionHandler.js

+            return;
+        }
+
+        // frame-based blink timing copied from orignal NPC code. might want timestamps instead?


No timestamps pls, because fps takes lag into consideration

Ah interesting! Thanks! The comment there was mostly a note-to-self about timing readability — maybe using a constant or two there could be helpful instead.

l0bster2 · 2024-10-28T18:19:31Z

handlers/TileMapHandler.js

+        const startFlagsInLevel = levelObjects.filter(levelObject => levelObject.type === ObjectTypes.START_FLAG);
+        const doorsInLevel = levelObjects.filter(levelObject => levelObject.type === ObjectTypes.DOOR);
+        const defaultStartFlag = startFlagsInLevel.find(startFlag => startFlag.extraAttributes?.defaultStartFlag);
+        const fallbackStartFlag = startFlagsInLevel[startFlagsInLevel.length - 1];


startFlagsInLevel length could be 0, and the index could be -1 then, or am i wrong?

You’re correct, yeah—in that case fallbackStartFlag will end up being undefined because there aren’t any start flags in the level. Currently this is handled further down below by printing a warning to the console when no suitable start location is found. In practice, this occurs when you have a level that is only accessible via door, and will trigger every time you access such a level in build mode.
This can actually be pretty common/easy to encounter, so maybe this behavior should be revised a bit to accept Doors as a fallback too. (Or to give Doors the same defaultStartFlag attribute as StartFlags, but that seems a bit overkill?)

l0bster2 · 2024-10-28T18:22:07Z

handlers/TileMapHandler.js

+        const startFlagsInLevel = levelObjects.filter(levelObject => levelObject.type === ObjectTypes.START_FLAG);
+        const doorsInLevel = levelObjects.filter(levelObject => levelObject.type === ObjectTypes.DOOR);


you could combine those filters into 1 array. if type === ObjectTyptes.START_FLAG || type === ObjectTypes.DOOR
as far as i've seen you don't need 2 separate arrays

Yep, at this stage I think they can probably just be handled identically. They will most likely be merged together in the final PR.

l0bster2 · 2024-10-28T18:23:09Z

handlers/TileMapHandler.js

+            if (destinationData.type === ObjectTypes.DOOR) {
+                destinationObject = doorsInLevel.find((door) => {
+                    return door.extraAttributes?.flagIndex === PlayMode.customExit.flagIndex;
+                });
+            } else if (!destinationData.type || destinationData.type === ObjectTypes.START_FLAG) {
+                destinationObject = startFlagsInLevel.find((startFlag) => {
+                    return startFlag.extraAttributes?.flagIndex === PlayMode.customExit.flagIndex
+                });
+            }


here you could have a longer if condition, and if flags and doors are in 1 array, you could iterate over this array and find flagIndex

l0bster2 · 2024-10-28T18:26:46Z

handlers/TileMapHandler.js

@@ -244,6 +246,44 @@ class TileMapHandler {
        }
    }

+    resetPlayerStartPoint() {


maybe it's WIP, but in general, there already is a function resetPosition in Player.js. not sure what your plan is with it. expand/delete it?

If I remember right, this is based on code in StartFlag (which is probably still in the main branch now) that seemed to be running every time a StartFlag is instantiated. The plan for that code seemed to be to find and adopt the start point for the level, so the appropriate position values are passed to the player. I refactored that so that it should only happen once when loading the level, but if that's redundant now, this is definitely worth trimming or deleting.

ok. you can also do it later on. if it works as it is now, it's also fine

l0bster2 · 2024-10-28T18:34:02Z

PlayLoop/PlayerInteractionHandler.js

+        let playerX = player.x;
+        let playerY = player.y;


i wouldn't create a variable for them, it just uses more memory, just use player.x and y down below

Sure, that’s fine. I was taught to cache one-time work before loops to optimize for speed, but honestly A) it’s just lookups here so any gain would be tiny and B) optimizing either way here seems unlikely to matter given such a small list. Deleting those lines will make the code a bit more concise without losing clarity though, so that’s a win.

yeah, i think cache makes sense, if you do some operation inside the for-loop each time. but here you are just creating a variable. so in this case at least, i would delete it

l0bster2 · 2024-10-28T18:35:08Z

PlayLoop/PlayerInteractionHandler.js

+        // should probably merge updateControl stuff into Controller and read from there instead?
+        // controls need to be updated every frame
+        this.updateControl('jump');
+        this.updateControl('up');
+        this.updateControl('down');


if would be awesome if we would have a "just pressed" variable in controller :)
but it's ok if not

Hah, I agree, but maybe we can add that in a future update. I think the unmerged ladder PR touched on some issues that would require a Controller update -- maybe it could be bundled into that?

nah, pls don't touch the ladder PR :D it's not a lot of code and there were lots of problems with it
if you want, you can create a separate PR in the future

l0bster2 · 2024-10-28T18:36:39Z

PlayLoop/PlayerInteractionHandler.js

@@ -0,0 +1,201 @@
+class PlayerInteractionHandler {


after a second read-through i have to say, i really like how you implemented this class 👏

l0bster2 · 2024-10-28T18:42:06Z

htmlRenderers/LevelEntrancePicker.js

@@ -0,0 +1,174 @@
+class LevelEntrancePicker {


Thx for creating a separate file for it!
That makes it more readable

l0bster2 · 2024-10-28T18:44:44Z

BuildMode.js

@@ -387,7 +404,7 @@ class BuildMode {

        const xPos = canvasOffsetLeft + (tilePosX * this.tileMapHandler.tileSize) - 120 - Camera.viewport.left;
        const yPos = canvasOffsetTop + (tilePosY * this.tileMapHandler.tileSize) + this.tileMapHandler.tileSize + 6 - Camera.viewport.top;
-        TooltipHandler.repositionAndShowTooltip("canvasObjectToolTip", yPos, xPos, heading, content)
+        TooltipHandler.repositionAndShowTooltip("canvasObjectToolTip", yPos, xPos, heading, content);


Thx for all the small clean ups as this :)

l0bster2 · 2024-10-28T21:55:45Z

PlayLoop/PlayerInteractionHandler.js

+            let xDist = target.x - playerX;
+            let yDist = target.y - playerY;
+            let dist = Math.sqrt((xDist * xDist) + (yDist * yDist));


just for pretty-code reasons: all those can be "const" as they don't change later on. It is also true for a lot of other usages of "let" that can be changed to "const" in your PR

Is this something you'd like to introduce as part of a "style guide" for the repo? I didn't notice much of a pattern in the codebase so far, but could adopt it going forward.
From a personal standpoint, I feel like I've annoyed myself just frequently enough with presumptuous const-ing (where later things change and it turns out I need a let instead) that I tend to use it pretty sparingly. I've also seen suggestions to avoid const strictly out of people being confused about the particulars of it (allowing mutability of a referenced object), but haven't been bit by that so far.

honestly i don't care that much about let/const^^ i would say if you are 100% sure that something is a const, try using it, but i won't block any changes of yours because of it

l0bster2 · 2024-10-28T21:58:52Z

htmlRenderers/LevelEntrancePicker.js

+        let levelIndex = parseInt(valueArray[0]);
+        let destType = valueArray[1];
+        let id = valueArray[2];
+
+        return { 
+            levelIndex: levelIndex, 
+            type:destType, 
+            flagIndex: id 
+        };


Suggested change

let levelIndex = parseInt(valueArray[0]);

let destType = valueArray[1];

let id = valueArray[2];

return {

levelIndex: levelIndex,

type:destType,

flagIndex: id

};

return {

levelIndex: parseInt(valueArray[0]),

type: valueArray[1],

flagIndex: valueArray[2]

};

Sure, this is just a style/readability choice. It seems like style guide entry number 2 for this repo might be something like "Don't declare local variables for available lookups. (Even if you personally feel like it's more readable with those declarations, [insert reasoning to not do so here].)"

i feel in this case lookups are really not necessary. you have an object with keys and values, and the keys already describe what the values do. imo there is no reason to also have variables before that. my reasoning is mostly keeping the code short if possible :)

l0bster2 · 2024-10-28T22:00:00Z

objects/Door.js

+        if (targetObjectType !== ObjectTypes.DOOR) {
+            return;
+        }
+        if (!reciprocatingLevelIndex) {
+            // (in this case, this door is probably the one who is reciprocating)
+            return;
+        }


Suggested change

if (targetObjectType !== ObjectTypes.DOOR) {

return;

}

if (!reciprocatingLevelIndex) {

// (in this case, this door is probably the one who is reciprocating)

return;

}

if (targetObjectType !== ObjectTypes.DOOR || !reciprocatingLevelIndex) {

return;

}

Ok. This is another style/readability choice. Maybe style guide entry number 3 would be something like "Consolidate conditionals." I feel like there should be a length limit before finding a good place to line-break or something though. Long, line-wrapping conditionals are like run-on sentences and can take a bit of dedication to correctly untangle.

yeah, i agree! with a certain amount of conditions, we should think about readability. but with 2 conditions, i would prefer keeping the code shorter

l0bster2 · 2024-10-28T22:06:58Z

objects/Door.js

+        let targetLevelIndex = customExit.levelIndex;
+        let targetObjectType = customExit.type;
+        let targetFlagIndex = customExit.flagIndex;


try not to reassign variables where there is no need for please. just use customExit.type where you need it down below f.e.

Sure, if you prefer. These local variables are just for readability. I feel like the naming here otherwise requires a broader awareness/understanding of other code and would be a bit harder to jump into.

ok, in that case the naming helps with readability, i agree 👍 let's keep it

l0bster2 · 2024-10-28T22:16:42Z

objects/Door.js

+        // overrides
+        this.lockedSpriteIndex = SpritePixelArrays.getIndexOfSprite(ObjectTypes.LOCKED_DOOR);
+        this.lockedSpriteYPos = this.lockedSpriteIndex * this.tileSize;
+        this.sound = SoundHandler.door;


i wouldn't save the sound in a variable. it's like 100kb, and a bunch of doors means, every door instance needs to hold that in it's memory. i would save SoundsHandler.door.key f.e. in the variable and when you need to play it, you can use
SoundHandler[instance.sound].stopAndPlay();
hope i made that clear
and same goes for FinishFlag.js

Doesn’t this just store a reference to the same Sound object that SoundHandler.door references? I don't think it would make a new copy each time.

You can test this out in the console:

const w1 = SoundHandler.win; const w2 = SoundHandler.win; console.log(w1.loaded); // prints: true console.log(w2.loaded); // prints: true w1.loaded = ":)"; // should mutate the object that both w1 and w2 refer to console.log(w1.loaded); // prints: ":)" console.log(w2.loaded); // prints: ":)"

Of course, if you make something assign SoundHandler.winDefault as the value, then THAT will create a new unique copy of the massive base64-encoded string, because string is a “primitive” type and not an object.

ah, yes. you are right :) let's keep it then

…ess (they're also generic enough to apply to other interactions)

…dd more robust method for creating select elements therein

…tom' changeable attribute form elements where we're generating bespoke form elements

…the future)

…ious commit (re: level entrances)

…hey now keep doing so, instead of pointing back to the original level they were copied from)

…ag; fix default new-level data to have the StartFlag always marked as the startingflag

…d tile in the default black background color

helioscope added 8 commits October 12, 2024 15:48

Add 2-way “Door” object, extending FinishFlag. Refactor various Finis…

8aff454

…hFlag and StartFlag-related code to be shared more cleanly with Door.

Merge branch 'main' into feature/doors

de5db94

Add PlayerInteractionHandler to handle key/button-triggered interacti…

656fc04

…ons and UI/arrow-markers (refactoring Door and NPC); add playerExitLevel method to handle level exits (refactoring Door and FinishFlag)

make door-activation-trigger user-configurable; add support for down …

337ac2b

…button as door trigger

add WASD support for down button

28d247a

add door sound

5fa3c8d

Merge branch 'main' into feature/doors (manually resolve BuildMode.js…

c34fff5

… merge conflicts)

darken door sprite

54617d8

helioscope marked this pull request as ready for review October 24, 2024 05:13

helioscope marked this pull request as draft October 24, 2024 05:14

l0bster2 reviewed Oct 28, 2024

View reviewed changes

Merge branch 'main' into feature/doors

416e5f9

l0bster2 reviewed Oct 28, 2024

View reviewed changes

l0bster2 approved these changes Oct 28, 2024

View reviewed changes

helioscope added 9 commits October 30, 2024 21:09

refactor the door-triggers constants to fit into the game export proc…

44173f0

…ess (they're also generic enough to apply to other interactions)

fix interaction trigger ids in previous commit; minor cleanup

af64c32

tidy up door activation-trigger selector in the build mode tooltip; a…

8b213e8

…dd more robust method for creating select elements therein

minor refactoring: switch from 'hidden' changeable attributes to 'cus…

a252876

…tom' changeable attribute form elements where we're generating bespoke form elements

allow doors to be fallback level entrances (or default entrances, in …

c936bf9

…the future)

assorted minor changes per style discussion in PR draft; fix for prev…

33ad7b0

…ious commit (re: level entrances)

fix pasting doors and flags that exit to the same level they're in (t…

000f1f7

…hey now keep doing so, instead of pointing back to the original level they were copied from)

revise default entrance logic to prefer picking the first starting fl…

bec677e

…ag; fix default new-level data to have the StartFlag always marked as the startingflag

darken the default door sprite, to make it look less like a foregroun…

a1e5315

…d tile in the default black background color

		const startFlagsInLevel = levelObjects.filter(levelObject => levelObject.type === ObjectTypes.START_FLAG);
		const doorsInLevel = levelObjects.filter(levelObject => levelObject.type === ObjectTypes.DOOR);

Add interactive "Door" objects. #12

Are you sure you want to change the base?

Add interactive "Door" objects. #12

Conversation

helioscope commented Oct 23, 2024 • edited Loading

l0bster2 commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0bster2 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helioscope Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helioscope Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0bster2 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0bster2 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helioscope Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helioscope commented Oct 23, 2024 •

edited

Loading

l0bster2 Oct 28, 2024 •

edited

Loading

helioscope Oct 29, 2024 •

edited

Loading

helioscope Oct 29, 2024 •

edited

Loading

l0bster2 Oct 28, 2024 •

edited

Loading

l0bster2 Oct 28, 2024 •

edited

Loading

helioscope Oct 29, 2024 •

edited

Loading