Grand Prix Menu - My code feels a little hacky

Greetings! I created some code for this UI:

The code just feels a little hacky to me.

If anything can be done a little differently, that would be great!
If everything seems normal in the code below, please let me know!

for CupName, CupData in pairs(GrandPrixInfo) do
	local GrandPrixButton = script.Example:Clone()
	GrandPrixButton.Name = CupName
	GrandPrixButton.CupAlias.Text = CupData.Alias
	GrandPrixButton.CupImage.Image = CupData.Icon
	GrandPrixButton.Parent = Frame.ContentBox
	GrandPrixButton.TextButton.MouseEnter:Connect(function()
		TweenService:Create(GrandPrixButton.UIStroke, ColorChangeInfo, {Color = Color3.fromRGB(254, 195, 9)}):Play()
		local IsShowing = true
		for MapID, MapName in pairs(CupData.Maps) do
			local MapData = MapInfo[MapName]
			Frame.Sidebar.Maps[MapID].Text = MapData:GetAttribute("Alias")
		end
		local MouseLeave: RBXScriptConnection
		task.spawn(function()
			while true do
				for MapID, MapName in pairs(CupData.Maps) do
					local MapData = MapInfo[MapName]
					Frame.Sidebar.MapImage.Image = "rbxassetid://"..MapData:GetAttribute("MapImage")
					Frame.Sidebar.MapImage.TextLabel.Text = MapData:GetAttribute("Alias")
					task.wait(1)
					if not IsShowing then
						break
					end
				end
				if not IsShowing then
					MouseLeave:Disconnect()
					break
				end
			end
		end)
		MouseLeave = GrandPrixButton.TextButton.MouseLeave:Connect(function()
			IsShowing = false
			TweenService:Create(GrandPrixButton.UIStroke, ColorChangeInfo, {Color = Color3.fromRGB(255, 255, 255)}):Play()
		end)
	end)
end
1 Like

I think the reason that the code feels a bit hacky is because of the looping within the MouseEnter connection + disconnecting the MouseLeave connection.

What I’d recommend is getting the loop out of the MouseEntered function and have a variable determine what maps the loop should be filtering through

local currentCupData = nil

for CupName, CupData in pairs(GrandPrixInfo) do
    local GrandPrixButton = script.Example:Clone() -- set properties
	GrandPrixButton.Name = CupName
	GrandPrixButton.CupAlias.Text = CupData.Alias
	GrandPrixButton.CupImage.Image = CupData.Icon
	GrandPrixButton.Parent = Frame.ContentBox

    GrandPrixButton.MouseEnter:Connect(function()
        currentHover = currentCupData
        ... -- tween service, set text labels etc
    end)

    GrandPrixButton.MouseLeave:Connect(function()
        currentHover = nil
        ... -- tween service, text labels 
    end)
end

while true do
    if currentCupData then
        ... -- go through currently hovered maps and show them
    end
    wait(1)
end
1 Like

This solution doesn’t seem to match what @BillB1ox is trying to do, or at least it’s not generally true. As the loop runs in another thread, the “map updates” will be run with some delay < 1 second after the button is clicked, which means a wrong map will be shown for some time. In a more general way, intervals are commonly necessary and alternative solutions do not always fit. The approach @BillB1ox is a valid solution. This is not exactly the way I’d do it though, but the idea is right. The only thing I find weird is that you’re disconnecting the MouseLeave connection in the loop instead of directly in the event listener, which avoids the second not IsShowing condition.

personally I’d use metatables, here’s a way to do it (could be much better if you used only 1 metatable to handle it all but I am lazy but lmk if u want me to do that)

local info = {
	hello = {maps = {'test', 'test2', 'test3'};};
	there = {maps = {'test4', 'test5', 'test6'};};
};

local mapinfo = {
	test = {icon = Color3.new()};
	test2 = {icon = Color3.new(1,0,1)};
	test3 = {icon = Color3.new(0,0,1)};
	test4 = {icon = Color3.new(1/2,1/2,1)};
	test5 = {icon = Color3.new(1/4,1/4,0)};
	test6 = {icon = Color3.new(0,1/4,1/4)};
}

local function count(t)local c=0; for _, x in next, t do c+=1;end;return c;end; local max = count(mapinfo);

local par = script.Parent;
local frame = par.frame;

for name, data in next, info do
	local button = par[name];
	local showing = false;
	local tracking_table = setmetatable({}, {
		__newindex = function(self, i, v)
			if v == 0 or not showing then
				print('stop map selection...');
				frame.BackgroundColor3 = Color3.new(1,1,1);
				return;	
			end;
			
			local show = v > max and 1 or v;
			
			local name = data.maps[show];
			local info = mapinfo[name];
			print('selected map:', name);
			frame.BackgroundColor3 = info.icon;
			
			task.delay(1, function()
				if not showing then
					return;
				end;
				
				self[i] = show+1;
			end);
		end;
	});

	button.MouseEnter:Connect(function() showing = true; tracking_table[name] = 1; end);
	button.MouseLeave:Connect(function() showing = false; tracking_table[name] = 0; end);
end;

preview:

This code is, conceptually, about the same as what they have done, except this one has a concurrency bug and the actual implementation is terrible. You’re using a metatable in place of a recursive function which is overkill, and even then it cannot work as expected because the state of a “thread” is shared by multiple threads.

Since I have not explicitly proposed a neat solution, I will now do so. task.cancel has lately rolled out into the engine and it will be a perfect fit for this case.

local thread

for cupName, cupData in pairs(GrandPrixInfo) do
    -- ...
    GrandPrixButton.MouseEnter:Connect(function ()
        if thread then task.cancel(thread) end -- handle this case as well
        thread = task.spawn(function ()
            -- your loop here
        end)
    end)

    GrandPrixButton.MouseLeave:Connect(function ()
        if thread then
            task.cancel(thread)
            thread = nil
        end
    end)
done

We can make the thread variable global because you are sure that there must not be more than one loop running at once. This, in particular, ensures that there will be no concurrency bug unlike what the precedently posted codes did.

2 Likes