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

Jump to bottom

Script library is not based on GetName (as AIs/GSes) #5662

Closed
DorpsGek opened this issue Jul 20, 2013 · 5 comments
Closed

Script library is not based on GetName (as AIs/GSes) #5662

DorpsGek opened this issue Jul 20, 2013 · 5 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

Zuu opened the ticket and wrote:

When OpenTTD scans for scripts/libraries, it will construct a string based on category, name and version which is used as a key when looking for a library (when a script tries to import it).

For AIs/GSs, OpenTTD uses the return value of GetName() in info.nut.
For libraries, OppenTTD should use GetName() too, however it uses CreateInstance() instead (which despite its name returns a string with the main class name).

This means that the return value of CreateInstance() gives the library name that scripts need to use when importing the library and not the GetName() name which is what we have communicated in the past.

For example, I have adopted a naming scheme where the main class in main.nut is always called MainClass in new scripts. To do so, CreateInstance() will return "MainClass" while GetName() returns the name of the ai/gs/library. The consequence of this bug is that the main class in main.nut need to have the same name as the library.

I've attached a patch which should fix the bug. However, unfortunately the library BinaryHeap rely on this bug by returning different values on GetName() and CreateInstance(). As many libraries and AIs/GSs rely on this library, it is hard to fix this bug cleanly without breaking most AIs/GSes on bananas.

Attachments

Reported version: trunk
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/5662
@DorpsGek
Copy link
Member Author

Zuu wrote:

Changes to documentation to reflect the current behaviour.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5662#comment12425

@DorpsGek
Copy link
Member Author

Zuu wrote:

Fix: import() has a separate parameter for version
Change: use -U 10 to give more context in the patch

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5662#comment12426

@DorpsGek
Copy link
Member Author

Rubidium wrote:

Could the taken name be toggled based on the version the actual GameScript/AI is defined compatible to?
Then for < 1.4 it'd return the old stuff, and for >= 1.4 the new variant.
I hope one can release a new version of BinaryHeap that doesn't trigger this issue, and maybe it's users. Then for newer GS/AIs we simply migrate to the more correct manner.

Or are we going to leave this as a documentation only fix? If so, please commit it.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5662#comment12761

@DorpsGek
Copy link
Member Author

Zuu wrote:

After further inspection. The following need to happen to fully fix issue:

- Queue.BinaryHeap, Queue.PriorityQueue and Queue.FibonacciHeap all needs to be get a new release where GetName return the same value as GetInstanceName does today.
- All AIs/GSes and libraries that use any of these libraries need to increase the imported version to the last one.
- In OpenTTD, if AI/GS report API version >= 1.4, use GetName() of library, otherwise use GetInstanceName()

As the libraries are fairly central, they are used by a large number of scripts. I therefore think that forcing this update upon a lot of authors is questionable. It will certainly be some authors that forget it and causes problems when a user tries to use the AI/GS on a newer OpenTTD version which uses GetName instead of GetInstanceName.

For that reason, I think that I prefer the doc update solution even if it doesn't solve the problem that the main class of libraries need to be named the same as the name used in Import.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5662#comment12780

@DorpsGek
Copy link
Member Author

Zuu closed the ticket.

Reason for closing: Implemented

In r26010

This comment was imported from FlySpray: https://bugs.openttd.org/task/5662

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) bug labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek