Skip to content

Simplify player heads#70

Merged
danthedaniel merged 5 commits intomainfrom
simplify-player-heads
Feb 8, 2025
Merged

Simplify player heads#70
danthedaniel merged 5 commits intomainfrom
simplify-player-heads

Conversation

@danthedaniel
Copy link
Copy Markdown
Member

When it was added I felt there was unnecessary JS complexity in handling player head rendering/updating.

  • Performs player head rendering with CSS
  • Downloads images using native <img> tags (should be optimal for performance)
  • Re-organizes some of the code in player_list.mjs

Overall this removes more code than it adds.

@danthedaniel danthedaniel requested a review from creesch February 5, 2025 01:12
@danthedaniel danthedaniel force-pushed the simplify-player-heads branch 6 times, most recently from b782894 to f4a0b8d Compare February 5, 2025 01:27
@danthedaniel
Copy link
Copy Markdown
Member Author

@creesch I could use your help testing with someone leaving and joining a server.

@danthedaniel danthedaniel force-pushed the simplify-player-heads branch from f4a0b8d to 269265d Compare February 5, 2025 04:15
@danthedaniel danthedaniel force-pushed the simplify-player-heads branch from 269265d to ad20743 Compare February 5, 2025 04:15
@danthedaniel danthedaniel force-pushed the simplify-player-heads branch from ca3a41c to 2347474 Compare February 5, 2025 04:29
@danthedaniel
Copy link
Copy Markdown
Member Author

I'm not sure about adding this to the PR as it's a little out there, but I have also considered performing declarative DOM building like this:

/**
 * Creates a new player element.
 * @param {StoredPlayerInfo} player
 * @returns {HTMLElement}
 */
#createPlayerElement(player) {
    // Create a new DOM element if none exists for the player.
    const playerElement = document.createElement('li');
    playerElement.setAttribute('role', 'listitem');
    playerElement.setAttribute('data-player-id', player.playerId);
    // Add click event to insert the player name into the chat input
    playerElement.addEventListener('click', () => {
        const cursorPos = this.#chatInput.selectionStart;
        const textBefore = this.#chatInput.value.substring(0, cursorPos);
        const textAfter = this.#chatInput.value.substring(cursorPos);
        this.#chatInput.value = `${textBefore}${player.playerDisplayName}${textAfter}`;
        this.#chatInput.focus();
        this.#chatInput.selectionStart = this.#chatInput.selectionEnd =
            cursorPos + player.playerDisplayName.length;
    });

    playerElement.appendChild((() => {
        const headContainer = document.createElement('div');
        headContainer.className = 'player-head-container';
        headContainer.title = `${player.playerDisplayName}'s head`;

        headContainer.appendChild((() => {
            const headImg = document.createElement('img');
            headImg.className = 'player-head';
            headImg.src = player.playerTextureUrl;
            headImg.setAttribute('aria-hidden', 'true');
            headImg.onerror = () => {
                headImg.src = '/img/steve.png';
            };
            return headImg;
        })());

        headContainer.appendChild((() => {  
            const headOverlay = document.createElement('img');
            headOverlay.className = 'player-head-overlay';
            headOverlay.src = player.playerTextureUrl;
            headOverlay.setAttribute('aria-hidden', 'true');
            headOverlay.onerror = () => {
                headOverlay.src = '/img/steve.png';
            };
            return headOverlay;
        })());

        return headContainer;
    })());

    playerElement.appendChild((() => {
        const nameSpan = document.createElement('span');
        nameSpan.className = 'player-name';
        nameSpan.textContent = player.playerDisplayName;
        nameSpan.title = player.playerName;

        // Specifically for aria labels show both playerDisplayName and playerName if they are different.
        if (player.playerDisplayName !== player.playerName) {
            nameSpan.setAttribute(
                'aria-label',
                `Display name: ${player.playerDisplayName}, Username: ${player.playerName}`,
            );
        } else {
            nameSpan.setAttribute(
                'aria-label',
                `Player name: ${player.playerDisplayName}`,
            );
        }

        return nameSpan;
    })());

    return playerElement;
}

Copy link
Copy Markdown
Collaborator

@creesch creesch left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me, I still need to test it though. Will do that in a bit.

One thing I did notice is that the lock and requestAnimationFrame is gone. Which should be mostly fine, except for maybe bigger servers with a lot more people online. Something I did try to keep in mind when I initially wrote this.

Going to ponder that a little bit, but it is probably fine. Once I have had a look at it running I'll complete the review.

@danthedaniel
Copy link
Copy Markdown
Member Author

danthedaniel commented Feb 5, 2025

I'm curious about your use of requestAnimationFrame(). My understanding is it will:

  1. Defer execution of its callback
  2. Wait until the browser is rendering the next frame
  3. Execute everything contained in the callback

It doesn't seem to be the performance fix it was intended to be. Perhaps sensible if there was some kind of batching of the work - build X elements, add them to the DOM, defer to next frame and repeat.

Edit: I am also suspicious of the similar use of requestAnimationFrame() in chat.mjs

@creesch
Copy link
Copy Markdown
Collaborator

creesch commented Feb 5, 2025

Yeah, your assessment of requestAnimationFrame() is fair, it might not even have been the perfect solution in its previous implementation. Though it will have helped a bit in preventing jank.
However, my remark wasn't just about the animation frame. Like I said, it was more broadly about how we handle potentially large player list updates.

So also the removal of the #isUpdating flag, though I suppose processing now will be quicker as we are not doing active fetch operations anymore. As I said, I just needed to ponder it a bit to make sure I am not missing anything.

@danthedaniel
Copy link
Copy Markdown
Member Author

My understanding of #isUpdating is it was just there because the function, while async, could be executed in parallel.

@creesch
Copy link
Copy Markdown
Collaborator

creesch commented Feb 8, 2025

All fair, looks good otherwise as I said. As far as this goes

I'm not sure about adding this to the PR as it's a little out there, but I have also considered performing declarative DOM building like this:

I don't have very strong opinions about this. On the one hand building an element first makes sense but doing it in the append callback like in your example does encapsulate it nicely where it is being added. If you want to do DOM building in this way I don't have an objection to that.

@danthedaniel
Copy link
Copy Markdown
Member Author

I'll hold off on the fancy DOM building for now. Honestly a lightweight component library might make more sense.

@danthedaniel danthedaniel merged commit 90a9000 into main Feb 8, 2025
@danthedaniel danthedaniel deleted the simplify-player-heads branch February 8, 2025 09:41
@creesch
Copy link
Copy Markdown
Collaborator

creesch commented Feb 8, 2025

Even a component library feels overkill for what this is. We don't have a ton of reusable components and the ones we have can easily be handled already.

creesch added a commit that referenced this pull request Mar 12, 2025
This release contains a variety of new features and improvements.  Things like support for newer minecraft versions, tab complete of usernames, hover events and a bunch more! 

# Main

## New

- Translations are now retrieved per chat message from the game at run time and stored with the message. (#84) Meaning that mod messages will display, messages will show in the language you will have set and that the mod now officially supports **1.21.1 and up** as messages will always have the correct translation for that minecraft version. 
- Support added for the `/tell`, `/msg`, `/w` and `/me` commands. (#82)   Note: This mod is never going to support all commands as this easily can be used for abuse. 
- Click events are now supported (#86)
- Tab completion of usernames (#78)
- Use "Jetbrains Mono" font for chat message/input/player names making it look more like minecraft chat (#80)
- Support for legacy color/format codes and multi-space formatting (#69)

## Fixes/enhancements 
- Make pingPatterns case insensitive (#63) 
- Added aria labels and use semantic elements (#58)
- Hover text now renders similar to in game (#85) and supports color code (#87)

# Technical
- Restructured front-end code to not only be modules but classes (#59)
- Simplify player heads rendering (#70)
- A variety of dependencies has been updated. 

# Various other 
For a complete overview see the [commit history](https://github.com/creesch/minecraft-web-chat/commits/main/)

# Screenshots 
![image](https://github.com/user-attachments/assets/91d4e140-c1a5-4fe2-ab74-9bee12ca0a7a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants