8000 Observers addon doesn't correctly handle components already in the component index · Issue #231 · Ukendio/jecs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Observers addon doesn't correctly handle components already in the component index #231

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you ag 8000 ree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Tazmondo opened this issue May 28, 2025 · 5 comments
Closed

Observers addon doesn't correctly handle components already in the component index #231

Tazmondo opened this issue May 28, 2025 · 5 comments
Labels
bug Something isn't working

Comments

@Tazmondo
Copy link
Contributor

Describe the bug

When using world:added, world:changed, or world:removed, if the component does not have an existing hook, but is already in the component index, then the hook component is added without actually overwriting the hooks table within the idr. This means the callback will never be run.

This does not appear to be intended behaviour.

Willing to PR a fix for this as well

Reproduction

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local jecs = require(...)
local patchObservers = require(...) -- observers addon

local world = patchObservers(jecs.world())

local component = world:component()
local entity = world:entity()

world:set(entity, component, 1)

local callbackRan = false
world:changed(component, function()
	callbackRan = true
end)

world:set(entity, component, 2)

assert(callbackRan)

Expected Behavior

Observers should work even if set after the component has been used.

Actual Behavior

Observers do not work if the component has been used already, unless it already has a hook set.
@Tazmondo Tazmondo added the bug Something isn't working label May 28, 2025
@Tazmondo
Copy link
Contributor Author
Tazmondo commented May 28, 2025 •
edited
Loading

for reference, this is what I've replaced the changed function with in my project (and similar changes for the other two)

world_mut.changed = function<T>(_: jecs.World, component: Id<T>, fn: (e: Entity, id: Id, value: T) -> ())
	local listeners = signals.emplaced[component]
	if not listeners then
		listeners = {}
		signals.emplaced[component] = listeners
		local function on_change(entity: Entity, id: number, value: any)
			for _, listener in listeners :: any do
				listener(entity, id, value)
			end
		end
		local idr = world.component_index[component]
		if idr then
			local existing_hook = idr.hooks.on_change
			if existing_hook then
				table.insert(listeners, existing_hook)
			end
			idr.hooks.on_change = on_change
		end
		world:set(component, jecs.OnChange, on_change)
	end
	table.insert(listeners, fn)
	return function()
		local n = #listeners
		local i = table.find(listeners, fn)
		listeners[i] = listeners[n]
		listeners[n] = nil
	end
end

@Ukendio
Copy link
Owner
Ukendio commented May 29, 2025 •
edited
Loading

Ur proposed solution does not handle when the OnChange hook has been set for a component that has not used for any entity yet.

@Tazmondo
Copy link
Contributor Author
Tazmondo commented May 29, 2025 •
edited
Loading

Aha, yeah good catch, this should deal with both cases now
Also tacked on a little change to the cleanup function that I think may be beneficial

Though looking at it again there's the possibility of:

  • Them using the component and it gets indexed
  • Then they set the OnChange hook
  • Then they call world:changed on the component

The hook they set would disappear, but I don't think it's a big deal since that hook would never have worked anyway?

world_mut.changed = function<T>(_: jecs.World, component: Id<T>, fn: (e: Entity, id: Id, value: T) -> ())
	local listeners = signals.emplaced[component]
	if not listeners then
		listeners = {}
		signals.emplaced[component] = listeners

		local function on_change(entity: Entity, id: number, value: any)
			for _, listener in listeners :: any do
				listener(entity, id, value)
			end
		end

		local idr = world.component_index[component]
		local existing_hook = if idr then idr.hooks.on_change else world:get(component, jecs.OnChange)

		if existing_hook then
			table.insert(listeners, existing_hook)
		end

		if idr then
			idr.hooks.on_change = on_change
		end

		world:set(component, jecs.OnChange, on_change)
	end
	table.insert(listeners, fn)
	return function()
		local n = #listeners
		local i = table.find(listeners, fn)
		-- tacked on this change as well, since it's not impossible for a cleanup function to be called multiple times
		if i then
			listeners[i] = listeners[n]
			listeners[n] = nil
		end
	end
end

@Ukendio
Copy link
Owner
Ukendio commented Jun 7, 2025 •
edited
Loading

Should be fixed now

https://github.com/Ukendio/jecs/blob/dab260f733c40552d9d221a7836e673f1e06f81d/addons/observers.luau#L201C4-L209C7

@Tazmondo
Copy link
Contributor Author
Tazmondo commented Jun 7, 2025 •
edited
Loading

Yep, I think that got it, works in my testing.

@Tazmondo Tazmondo closed this as completed Jun 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Tazmondo @Ukendio
0