Skip to content
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.

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

Already on GitHub? Sign in to your account

Fix AdvDupe2 support, allow all tools (except ones causing #60) to be used on textscreens #90

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stepa2
Copy link

@stepa2 stepa2 commented Mar 8, 2024

It is already possible to area-copy textscreen and paste it, but that previous broke it.

I tried detouring ENT:SetNotSolid on textscreens, because it is the cause of #60, but it not worked, so I check for ENT:IsSolid() == false each 5 seconds.

stepa2 added 2 commits March 8, 2024 19:36
Now any other tool can be used on textscreens.
@Cherry
Copy link
Owner

Cherry commented Mar 8, 2024

Thanks for the PR. I'm not entirely understanding what this is trying to fix though, can you explain further? Is it solely that the AdvDupe2 tool can't be used on Textscreens right now?

A few other questions:

  • This seems to assume duplicator is always present? What happens when it's not? Does this fail?
  • The behaviour change here seems to be very different. Previously, I would only allow the sammyservers_textscreen, textscreen, remover, and permaprops tools to interact with textscreens, to prevent weird edge-cases when people would weld them, attach ropes to them, etc. Why was this removed? It also looks like the hook.Add call after was left in, so this would error now since textScreenCanTool was removed in this PR.
  • Is there no other way to add compatibility with AdvDupe2 without introducing code that runs in Think?

I don't think allowing every tool to interact with textscreens is a good idea. Over the years, I've seen countless incompatibilities, thus why there is a very strict allowlist. I'm happy to add to that allowlist though if it makes sense.

@stepa2
Copy link
Author

stepa2 commented Mar 8, 2024

  • I was able to duplicate textscreen with AdvDupe2 area copy (Shift+RMB), but it broke up on pasting (self.lines became nil, probably).
  • duplicator is a base GMod library (https://wiki.facepunch.com/gmod/duplicator).
  • ENT:Think hook is not related to AdvDupe2 fix, it is to disallow making textscreens unremovable by admins by using Fading Door (see Textscreen can be made into a fading door. #60, the title is wrong).
  • I want to allow parenting textscreens to different stuff to, e.g., put some text on a vehicle. I don't understand what is wrong with roping and welding textscreens (especially if auto-freezing will be disabled).
  • Why did you added whitelist for some compatible tools and not blacklist for all incompatible ones?

@stepa2
Copy link
Author

stepa2 commented Mar 8, 2024

Previously, all tools but Remover and PermaProps were unusable due to whitelist.

@Cherry
Copy link
Owner

Cherry commented Mar 8, 2024

Thanks for the info! I didn't realise duplicator was a base GMod thing, sorry - I've been out of GMod for a while 😅. I'm very happy to accept the duplicator fix, thank you for the addition!

With the other tools, I think this should really be a wider discussion and not something addressed in this PR. This tool has always used an allowlist because that's significantly easier to maintain - new tools are made all the time, as well as custom ones in specific servers. I don't really have the time to manually check all of them to blocklist ones that are problematic. I have seen countless issues over the last decade with all kinds of other tools paired with textscreens, from render problems, crashes, performance concerns, etc.

I'd encourage you to open an issue about additional tool support so this can be discussed in more detail for the future.

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