search

aws-samples / amazon-sumerian-hosts

Amazon Sumerian Hosts (Hosts) is an experimental open source project that aims to make it easy to create interactive animated 3D characters for Babylon.js, three.js, and other web 3D frameworks. It leverages AWS services including Amazon Polly (text-to-speech) and Amazon Lex (chatbot).
MIT No Attribution
183 stars 82 forks source link

Messenger fix #83

Closed runpiw closed 2 years ago

runpiw commented 2 years ago

What

This change merges the inherited Messenger class functions in three and babylon into their corresponding HostObject class so that proper methods can be called at runtime.

Why

Core Messenger class usage used to be dynamically resolved to child Messenger class at build time which is not doable anymore with package split, so this change just combine the child inheritance into the HostObject class to make it work again. The problem with the pointOfInterest in babylon example is not working correctly is due to child Messenger class in babylon is not used. While the core Messenger relies on the id passed in to differentiate events belong to different object, our example actually has same id for both host causing two hosts receiving the event meant to be consumed by only one host. The static GlobalMessenger which still being an instance of core Messenger still works due to its using a generated UUID for tracking the event host.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Krxtopher commented 2 years ago

Please don't merge this until I finish my review. I'm testing a couple things.

runpiw commented 2 years ago

I don't see any unit tests that validate this fix. Whenever we find a bug we should strive to implement a failing unit test/tests that reproduce the bug. Only then should we implement a fix.

These unit tests serve allow us to understand the bug clearly, demonstrate when we've actually fixed the bug, and—most importantly—will catch any regressions in the future related to this bug.

Please implement unit tests for both the Babylon and Three HostObject classes that fail under the original implementation and pass under the new implementation.

Verify unit tests are added properly as per Kris's last comment.

Added unit tests for testing HostObject in babylon and three and make sure hosts with same id does not interfere each other with message system